3 column preview card challenge

Community feedback

• 

  Raymart Pamplona•16,100

  @pikapikamartPosted about 3 years ago

  Hey, awesome work on this one. Layout in desktop looks great, though it is not responsive. Right now before going into mobile breakpoint, the layout has shrunk which is not great, your mobile breakpoint is too late. I suggest starting from mobile-first workflow to avoid this kind of scenarios.

  Ken already gave great feedback on this, just going to add some suggestions as well:

  • Always have a main element to wrap the main content of your page. On this one, the .container should be using the main instead of div.
  • Each car icon is just a decoration. Decorative image must be hidden at all times for screen-reader users by using alt="" and extra aria-hidden="true" attribute on the img tag.
  • Also when using alt attribute, avoid using words that relates to "graphic" such as "icon" and others. An img is already an image/graphic so no need to describe it as one.
  • Avoid using multiple h1 on a page, use only at least 1 per page so change those into other heading tags.
  • A page must have a single h1 on a page. Since there are no text-content that are visible that could be h1, you will make the h1 screen-reader only text. Meaning this will be hidden for sighted users and only be visible for screen-reader users, search about sr-only stylings and see how it is used. The h1 text should describe what is the main content is all about, this h1 would be placed as the first text-content inside the main element. Have a look at Grace's solution on this one inspect that layout and see the usage of the h1 as well the stylings applied to it.
  • Those learn more are better using a tag since on a real site, it would be a link to "learn more" about the car.

  Aside from those, great job again on this one.

  Marked as helpful

  2

  ---

  

  othman•120

  @othmanbenhamdounePosted about 3 years ago

  @pikamart thank you so much your feedback is very helpful

  1
• 

  Ken•935

  @kenreibmanPosted about 3 years ago

  Good work! It seems like you had trouble centering your card within your body. For future projects like this there is a quick and easy way to center your container. In your body tag for CSS if you put:

  body {
    display: flex;
    justify-content: center;
    align-items: center;
    min-height: 100vh;
  }
  

  The child content (in your case it would be .container) will automatically be centered in the middle your page.

  I recommend you to start a media query to change into the "mobile version" of the card under 1000px. The contents seems to start spilling out around 380px and you finally change the layout at 375 pixels. I know the challenge just tells you to do 1440px and 375px but I challenge you to make it a habit to make it responsive for all other screen sizes in between!

  Marked as helpful

  2

  ---

  

  othman•120

  @othmanbenhamdounePosted about 3 years ago

  @lmaoken I really appreciate your feedback thank you so much bro

  0