Four card feature section

Community feedback

• 

  T

  Grace•31,070

  @grace-snowPosted 2 months ago

  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-acnPosted about 2 months ago

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

  0

  

  T

  Grace•31,070

  @grace-snowPosted about 2 months ago

  @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-acnPosted about 2 months ago

  @grace-snow noted with thanks!

  0
• 

  ავთანდილ საგინაშვილი•150

  @DeeFyB707Posted 2 months ago

  nice job

  0