Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found

Submitted

Four card feature section

thamu-acn 200

@thamu-acn

Desktop design screenshot for the Four card feature section coding challenge

This is a solution for...

  • HTML
  • CSS
1newbie
View challenge

Design comparison


SolutionDesign

Community feedback

T
Grace 29,310

@grace-snow

Posted

Hi there,

Well done on your first draft. I hope this feedback is helpful.

  • you need to remove the header from this and move all of that content into theb main landmark. The header landmark isnt meant to hold page specific content like headings. It has a banner role and that is specifically for primary repeating content across a site.
  • what you have as a paragraph and then h2 is meant to be read all together. Thats how it makes sense. That tells you it should be one heading element, not two elements. And as it is the main heading for the page, it needs to be a h1. Remember you can split half of it to display on its own line using a span or strong tag set to display block.
  • The images in this are decorative. That means the alt should be empty with no content or spaces alt="".
  • it looks like youre trying to use BEM naming here. That's great and you're very close. But the modifiers arent quite right on the cards. They should be for example card--supervisor not card-grid--supervisor. You are modifying the cards, not the grid.
  • there's no need to load the css reset as a separate file. Just pop it at the start of your css file. That's better for performance.
  • This all looks very narrow and strangely off center for me on mobile. That's caused by the width 80% (far too small) and lack of margin-inline set to auto.
  • this would be much better if you styled it mobile first. The card grid would stack by default on mobile and set the gap between cards. Then you only need to add the grid-template columns, rows and placement on larger screens.
  • When you use media queries, define them in rem or em not px. This will allow the layout to scale well for all users, including those with a different text size.

Marked as helpful

0

thamu-acn 200

@thamu-acn

Posted

@grace-snow Thank you very much for such detailed feedback I have implemented all your recommendations.

0
T
Grace 29,310

@grace-snow

Posted

@thamu-acn well done, fast work!

A small thing - You don't need a br in the heading in my opinion. (That will be announced by some screen readers as "break".) I'd just set the span part as display block and it would then sit on its own line.

0
thamu-acn 200

@thamu-acn

Posted

@grace-snow noted with thanks!

0

Please log in to post a comment

Log in with GitHub
Discord logo

Join 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