Design comparison
Community feedback
- @FlashdanielPosted 5 months ago
Nice work. I like to point a few things:
- Use headings h1, h2 ... h6 orderly, not h2 then h2 again like in your code.
box-sizing
property is not inherited, so adding it to the body element only affects the body. if your thought was to all the elements. Do this
*, *::before, *::after { box-sizing: border-box; }
You can make your site more responsive by changing the
.parent { grid-template-columns: 1fr; }
to.parent { grid-template-columns: repeat(auto-fill, minmax( your, 1fr); }
.Learn to make use of the
repeat()
function, instead ofgrid-template-columns: 1fr 1fr 1fr;
Dogrid-template-columns: repeat(3, 1fr);
. And don't usegrid-template-rows
if it's not necessary like in your case, allow Auto-placement in grid to make that decision for you.Marked as helpful1@kendo-desuPosted 5 months ago@Flashdaniel Thank you for your comment and all the tips, I find them very helpful as I'm a complete beginner who has just started learning to code last month. I understand all the points you raised except the
repeat(auto-fill, minmax(your, 1fr);
part. Is it possible for you to elaborate more on that?0@MattPahutaPosted 2 months ago@kendo-desu
I'm no Grid guru but I have found Kevin Powell's content immensely useful for tackling most every layout challenge I've come across. He has a lot of good stuff on Grid and flexbox, but here's a quick and simple video that I think will send you on the right path. Cheers!
1 - @martbudrPosted 5 months ago
The solution works both on desktop and mobile. The layout looks good on different screen sizes. The code is written in a readable way. The solution looks very similar to the design.
1 - @MattPahutaPosted 2 months ago
Hi there. Great job on completing this challenge! You've matched the design comp quite well and achieved the layout using CSS Grid, which is the technique this challenge is designed to highlight. I also appreciate you correctly identified the icon images as being purely decorative and gave them appropriate alt values (empty values).
As a previous reviewer mentioned, headings should always go in order to communicate the structure of the content correctly to assistive technology and bots. This design calls for one
h1
tag and fourh2
tags.Here are a few other things I flagged for you:
- Back to the main page heading, this should be one
h1
split over two lines. You can wrap half in astrong
tag or styledspan
with display set to block. - You're using a lot of IDs in your html where you should be using classes. It's generally best practice to style elements using classes rather than IDs or by element tag. IDs are better suited for targeting elements via JavaScript, page navigation, linking labels with inputs, and a few other use cases. There is no need for IDs in this particular challenge.
- The cards don't need an extra wrapping div. One div within your main tag is plenty. You should always strive to keep your html as simple as possible.
- Get in the habit of using a modern CSS reset for all your projects. Andy Bell and Josh Comeau both have excellent resets you can google and use. Place it at the beginning of your styles file.
- Font sizes should always be set using rem units, not pixels. You're using a mix of both here, so you'll want to be consistent throughout your code.
- Media queries should also be defined using rem or em units, not pixels.
Again, nice work here and good luck moving forward!
0 - Back to the main page heading, this should be one
Please log in to post a comment
Log in with GitHubJoin our Discord community
Join thousands of Frontend Mentor community members taking the challenges, sharing resources, helping each other, and chatting about all things front-end!
Join our Discord