Responsive cards feature section using CSS

Community feedback

• 

  T

  Grace•31,090

  @grace-snowPosted 5 months ago

  It's a real shame you've used flexbox for this layout when it is so clearly a css grid challenge. I recommend you try to refactor this using css grid to stretch the skills it is designed to stretch, using a simpler html structure where the 4 cards are 4 direct children of one grid template.

  Other points to note:

  • the main heading is one h1 split over 2 lines. Wrap half in a strong or span set to display block.
  • try not to skip heading levels. Headings should always go in order to communicate the structure of the content correctly to assistive tech and bots. If the main heading is a h1, the cards should all have h2s.
  • the cards don't need an extra wrapping div. Keep the html as simple as possible.
  • for the top content give it a max width in rem so it scales correctly for all users no matter what their text size settings. This would be instead of setting an explicit width in px.
  • get into the habit of including a full modern css reset at the start of the styles in every project. Andy Bell or Josh Comeau both have good ones you can look up and use.
  • if making the body into a flex container don't forget to set its direction to column.
  • Font size must never be in px. This is really important! All of the font sizes look too small in this compared to the design too, as has already been mentioned.
  • the cards should not have a width. Their width will be controlled by the grid template when you add it and be 1fr (the grid itself should have a max width in rem).
  • do not add hover effects to non-interactive elements! That would imply interactivity but there is nothing interactive shown here. -'the icons should have margin left of auto not an explicit px value.
  • mobile styles should be the default. You should then be adding a min-width media query defined in rem or em at the point where there is room for the layout to change. See https://fedmentor.dev/posts/responsive-meaning/ for more info.

  0
• 

  yousef•70

  @yosefHeshamPosted 5 months ago

  well done doing this design. notes:

  • consider use semantic tags like <header> and <section>
  • font size is smaller than the design

  0

  ---

  

  T

  Grace•31,090

  @grace-snowPosted 5 months ago

  @yosefHesham there is no header shown in this design and no content that would be appropriate to use a header.

  Using section elements for the cards would be fine but brings no additional benefits at all in this.

  The important thing is the page has included content inside a main landmark, which is essential!

  0

  

  yousef•70

  @yosefHeshamPosted 5 months ago

  @grace-snow I think the title might be an appropriate content to be used inside header.. or maybe not. Anyways, the feedback is not attacking you personally, just a matter of opinions. Have a nice day :)

  0

  

  T

  Grace•31,090

  @grace-snowPosted 5 months ago

  @yosefHesham it's really not a matter of opinion on this one. The header element is a landmark with a "banner" role. That means it is for repeating content across a site (like a logo or navigation). It must never include page-specific content like a h1. It's actually very important for accessibility and SEO.

  1

  

  yousef•70

  @yosefHeshamPosted 5 months ago

  @grace-snow Great, thanks

  0