Martin
@ringmAll comments
- @Albertoflj@ringm
Hey! Amazing job, the site looks exactly like the design. Congatulations!
Regarding your question: if you define your grid to be 1 column wide, all the child elements of the grid will fit like you want.
.container { display: grid; grid-template-columns: 1fr; }
Keep up the good work!
- @mandeephub@ringm
Looks good! Congratulations on completing your fist project.
My only suggestions would be vertically centering the content for tablet/desktop devices.
Looking forward on seeing your next projects!
- @luisdevworks@ringm
Looks good! Just remember for accesibility reasons, you should have a
<label>
for every<input>
.Your form should be structured like this:
<form action="/action_page.php"> <label for="firstName">First Name</label> <input type="text" name="firstName" id="firstName" value="First Name"> </form>
More info on labels here
Happy coding!
- @Nazarkon@ringm
Hey Nazar, congratulations on submitting your first challenge. It's looking good! I have some suggestions to help you improve it:
- instead of using fixed margins on the
.content-wrapper
, consider using something likewidth: min(90%, 1100px);
andmargin: 0 auto;
. With these two you'll get a fluid and centered container. - for the
header
, remove theflex: wrap;
. Instead, tryflex-direction: row;
and when it gets too narrrow you change it toflex-direction: column;
- same as the header, remove the
flex-wrap
from.gallery-wrapper
and also remove the fixed width on it's child elements, let them be fluid and stretch to the available space. once the screen get's too narrow, you can change theflex-direction
on.gallery-wrapper
tocolumn
. - also, the HTML report is yelling at you because you didn't include any
h2-h6
tags inside the<section>
. To avoid this problem, you can either change the sections elements for regular divs or include headings and hiding them.
Keep at it! I'm looking forward to your next project!
- instead of using fixed margins on the
- @drametoid@ringm
Hey Shubham, the site looks good! Couple suggestions to polish it:
- Consider setting the background image in css through the
background
property. There you can also set thebackground-size
andbackground-position
to place it exactly where you need. - For a responsive layour, try the following:
.wrapper { display: flex; height: 100vh; width: 100vw; } .bg-image { background-image: url(../images/bg-mobile.svg); background-color: #0c122c; background-repeat: no-repeat; background-size: cover; } .container { width: min(90%, 860px); margin: auto; } .row { display: flex; flex-direction: column; } .col-30, .col-70 { width: 100%; border: 1px solid red; } @media (min-width: 600px) { .row { flex-direction: row; } .col-30 { width: 30%; } .col-70 { width: 70%; } }
<div class="wrapper bg-image"> <div class="container"> <div class="row"> <div class="col-30">Fylo block with logo and buttons</div> <div class="col-70">Fylo block with the bar and storage info</div> </div> </div> </div>
The class bg-image should be used to place the background image.
That should take care of the main layout for mobile and desktop, I hope it helps! Let me know if you have any doubts with the code.
- Consider setting the background image in css through the
- @En-Jen@ringm
Looks amazing! Exactly like the design! Congratulations :)
- @allisonkbates@ringm
Hey Allison! Great job, the site looks really good! Well done.
Regarding your question, I would suggest trying a layout with flexbox like this:
.container { width: min(90%, 1100px); margin: 0 auto; } .row { display: flex; flex-direction: column; } .col { width: 100%; } @media (min-width: 600px) { .row { flex-direction: row; } }
<div class="container"> <div class="row"> <div class="col">H1 and text</div> <div class="col">Ratings</div> </div> <div class="row"> <div class="col">Comment 01</div> <div class="col">Comment 02</div> <div class="col">Comment 03</div> </div> </div>
This would take care of the overall layout both for mobile and desktop, I hope it helps!
Happy coding!
- @alexcamachogz@ringm
Hey Alex! Great job! The site looks great. I have tiny suggestions to polish it.
- Center the main container so the content sits nicely on the middle of the screen (in your case, maybe adding
margin: 0 auto;
on the<main>
. Although as a better practice, I would suggest wrapping everything in a<div>
and give it acontainer
class. - Your
<input>
should have<label>
, that's why you're having html issues on your report. Also, consider wrapping your forms with a<form>
, it's a better practice from the semantic point of view
An example would be:
<form> <label for="firstName">First name:</label> <input type="text" id="firstName" name="firstName" value="John"> <label for="lastName">Last name:</label> <input type="text" id="lastName" name="lastName" value="Doe"> </form>
Just tiny things! Overall it's a great job! Keep at it!
- Center the main container so the content sits nicely on the middle of the screen (in your case, maybe adding
- @theAspiringDev1@ringm
good job! overall it working pretty well. I'm also using tailwind for my latest submissions and I feel it really improved my workflow.
Tiny suggestions:
- You're missing the
alt
attribute on theimg
tags, which doesn't create any problems in the visual side, but for accesibility purposes you should always include it. - I feel the background image would look much better if it would cover the entire size of the viewport. On mobile and tablet looks fine, but on desktop looks weird. Also consider giving a
max-width
to the container so it doens't stretch infinetly ;)
Awesome work! Looking forward on what you build next!
- You're missing the
- @chri55@ringm
looks amazing! really neat animations and the responsiveness is sweet.
tiny suggestion: maybe adding a
max-width
to the container? I'm opening it on a 4k monitor and, although it's not breaking (which is really nice) I feel is stretching too much, and the font display at that size is too small.Other than that, it's great! Keep at it!
- @Aahil13@ringm
Hi Onyeanuna! Great job completing this challenge, it’s looking really good!
However, there’s an issue with the main img, on small/medium devices it’s ok, but on large devices it grows extremely big and brakes the design! A fast duck-tape solution could be setting a
max-width
so it doesn’t stretch indefinitely. Another solution would be to make some layout adjustments to the whole layout for bigger screens with media queries.Keep coding!
- @KrishnanPraveen@ringm
Looking great! I think the only thing missing is
box-shadow
on the main component.Here's a box-shadow generator that can help you achieve exactly the look you want with ease: https://www.cssmatic.com/box-shadow
Great work!
- @faraz343@ringm
Hey Faraz! Greay job on completing this one. The site looks really good. I have some suggestions to take it a step further:
- Accesibility Issues: your img tags are missing the ‘alt’ attribute, which specifies an alternate text for an image, if the image cannot be displayed. Also, you are using the backslash character () for the path of the imgs. You should use the forward slash symbol (/).
- Layout: I think the site would look much better if you make the background img stretch the whole width and height of the viewport. Right now you’re working with fixed units which is something you almost never should do in responsive design, its making and undesirable thick white frame all around the site. I would suggest placing the background image as a background img on a wrapper div, and then change the img according to the device on CSS.
- Carefull with the phrase “GB Left”, it has the wrong typography.
- You could get a much precise layout if you use flexbox for the general layout as well as the individual components. Try replacing your absolute positioned elements and the floats for some flexbox directives! Let me know if you have any questions, I’m happy to help! I leave you my solution to this challenge here in case you want to take a look at the code for some ideas.
Happy coding!
- @Raymond-ap@ringm
Hey Reymond, congratulations on finishing this project, looking really good! I have some few suggestions that I think could improve the site:
- The font is missing :P
- Your testimonial imgs are stretching becasue you set a fixed height on
.profile
. - Adding a little more padding to the elements to separate them would improve the layout.
- I think the name and profession are too big. The font-size should be smaller than the paragraph.
- Adding some transition animation between testimonies would look sooo cool! Are you up for the challenge? Here's my solution if you want to check it out.
That's all I can think of right now! Overall it looks great and it' working just fine. Excellent job!
Marked as helpful - @Habeebullahi01@ringm
Hey Habeebullahi, great work! I think the overall layout of the site looks good. And it's also responsive, congratulations.
I checked your code and I think you could achieve a better result using flexbox instead of floats. You could try something like this:
.cards { display: flex; flex-direction: column; align-items: center; } @media (min-width: 600px) { .cards { flex-direction: row; } }
That should do the trick! In small devices your cards will display one below de other, and when the screen hits 600px size (you can change that if you like) they will display like the big screen layout.
Also, I leave you here a short article that explains all the different ways you can vertically center an element.
I hope it helps! Good luck and happy coding :)
- @jomefavourite@ringm
Hey Jome! Excellent job, the site looks great!
Regarding the theme toggle: there's a cleaner way to do it if you use css variables. Here's a video that explains it really well.
I leave you my solution to the challenge, where I implement this method.
Keep up the good work!
- @amidabrian51@ringm
Hi Brian, congratulations on submitting this project. I just happened to take a quick look yesterday and now it’s looking much better! I downloaded your project to take a look into the code and see how you can make it even better, here are my suggestions:
- First of all, you’re getting an HTML issue because you’re using the
section
element and not giving it ah2-h6
element as a child. Thesection
element should represent a standalone section inside the HTML structure. This is not the case in your HTML, since the.company-heading
and the.community
are not standalone sections, they are both parts of the same content. I would suggest changing them for regulardiv
elements and using thesection
element as a container for both. Example:
<section class="container"> <div class="company-heading"> </div> <div class="community"> </div> </section>
- Now, another issue you have right now is that the layout is not centered. If you follow allong with my example, you can center and give it a
max-width
with very few lines of code. First, remove themax-width
from thebody
element. We’re gonna use the.container
, as the... well... the container of the whole design ;P
.container { width: min(90%, 1440px); margin: auto; }
The
min()
function is extremely usefull and flexible. Basically what it does is to analyse the 2 values you pass, and use the minimum. In this case, we’re passing 90% and 1440px. What it will do is ask ‘what’s the smallest value? 1440px or 90% of my current width?’ If 90% of your current width is less than 1440px, it will use that as the width on the container. This is a very cool way of having a flexible width on the whole layout without worrying about any breakpoints. When the screen size get’s bigger, it will reach a point where 90% of the viewport will translate to a number that is bigger in px than 1440, the function will say ‘hey, 1440px is smaller than 90% of the current viewport, so I’ll stuck at 1440px’. It will basically give amax-width
to your design and prevent it from stretching endlessly. For more information on the min function click here.Lastly,
margin: auto;
will center you design (horizontally and vertically). If you don’t see it veritcally centered, try giving the parent element of.container
a height of 100vh and that will do the trick ;)- Ok, now that we have the layout centered and we gave it a
max-width
, let’s go into the inner components: the image and the text. Here you got it right, the easiest approach would be to setdisplay: flex;
in the parent element. Following my example that would be on.container
.
.container { width: min(90%, 1440px); margin: 0 auto; display: flex; }
And that should do it! But we’re not there yet. I can see that you struggle a little trying to match the design. If you’re following my code, I’m gonna ask you to take a leap of faith and just delete some styles you wrote.
• Remove all the
padding
from the.header-title
,.company-heading
and.community
• Remove thewidth: 70vh;
from the.huddle-content
• Remove thewidth: 50%:
of.company-heading
and.community
After doing that you’ll see that the content is sitting on the middle of the screen (in big screens). To ensure they’re the same size and since they're flex items, instead of using
width
, we give each of them (.company-heading
and.community
) aflex: 1;
value. Now the should be the same size, which is exactly what we want. To separate them you can addmargin-left
to.community
ormargin-right
to.company-heading
(5% should look good). Finally, to center the content you can also addalign-items: center;
on.container
.Finally, for the mobile breakpoint. All you’d have to do is set
flex-direction: column;
on.container
and remove either themargin-left
ormargin-right
you set on it’s child elements to have it looking like the design.And that should give you the basic behaviour you want for the whole layout of the page. If you’re still seeing some issues, try removing all the padding’s and margin’s u have on the media query. A lot of times it’s easier to find the bugs when u start removing things.
Sorry if it got too long. I hope it’s not confusing! I really like the job you did, the styles are almost there, I think the main issue was the overall behaviour of the layout, which is what I’ve tried to help you with.
Let me know if you tried my approach and most importantly, if it worked! I’ll be happy to help if you encountered any issues.
Keep up the good work! Happy coding.
- First of all, you’re getting an HTML issue because you’re using the
- P@okowu@ringm
Hi o.kowu, congratulation on finishing this one. It looks really good! You really nailed the desktop and mobile versions of the site.
However, you could really improve it if you made it responsive (that is: make the components automatically adjust their size based on the screen size available). Right now you have a breakpoint that changes the layout from desktop to mobile, but what about the sizes of the devices in between? A tablet for example.
Also you have an issue with the background. You set a max width on the body element, which is making the whole page appear at the top left screen of the browser. This is not ideal. The bg image and the color should fill the entire browser window, and the components be centered.
As a general rule, is almost never a good practice to work with fixed width's and height's. Start by removing them and see what happens!
If you want to center an element, a quick trick is to set it's parent to
display: flex;
and the give the element you want to center amargin: 0;
. That would do the job.A possible solution for the bg and centered content would be:
/*this class should be on a div that wraps all the rest of the elements*/ .bg { width: 100vw; /*flexible unit: this way you make sure you fill the entire width of the viewport*/ height: 100vh; /*flexible unit: this way you make sure you fill the entire height of the viewport*/ background: url('./images/bg-desktop.svg') no-repeat bottom, hsl(229, 57%, 11%); /*bg color and img on the same definition*/ } .container { height: 100%; display: flex; } .content { margin: auto; }
I hope I was clear enough, give it a try and if doesn't work let me know I'll help you polish the rest ;)
Happy coding!