3-column preview card component challenge

Community feedback

• 

  Raymart Pamplona•16,100

  @pikapikamartPosted about 3 years ago

  Hey, awesome work on this one. Layout in desktop looks great, it is responsive and the mobile state looks great.

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

  • The body does not need the overflow prop, you are using this right to hide the horizontal scroll, to prevent this on the first place, don't add width: 100vw as this adds an extra horizontal scroll since this doesn't account the scrollbar's size and it just create a horizontal scrollbar like what you did on the main.
  • Use width :100% on the main.
  • Avoid using height: 100vh on a large container like the main tag as this limits the element's height based on the remaining screen's height. Instead use min-height: 100vh, this takes full height but lets the element expand if needed.
  • Your aria-hidden is not using a proper quotation mark, check it out use aria-hidden="true" right now it is using aria-hidden=""true” making the alt not work properly.
  • Avoid using multiple h1 on a page, use only at least 1 per page so change those into other heading tags.
  • 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 the layout, see the usage of the h1 as well the stylings applied to, copy those since you will use that a lot (sr-only).
  • Do not directly type the wordings as uppercase on the markup, if you do this, screen-reader will read the text letter-by-letter and not by the wordings. Use only the lowercase version to write in the markup and instead use text-transform: uppercase on it.
  • Using a tag for the learn more will suit more rather than button, since on a real site, this would be a another page for the user to "learn more" about the specific car.

  Aside from those, great work again on this one.

  Marked as helpful

  1

  ---

  

  Chris•250

  @chintriagoPosted about 3 years ago

  @pikamart Man thanks so much for your comment! Feedback like this really helps with my learning I appreciate it!

  1
• 

  P

  Ken•4,915

  @kens-visualsPosted about 3 years ago

  Hey @chintriago 👋🏻

  I have some suggestions for the project.

  • The car icons, should have aria-hidden="true”, because they are for decoration. You can read more about aria-hidden here.
  • Also, I suggest implementing :hover state, which you can find in design folder active-state image, after you implement it you can also add transition: all 0.2s; to the button and the links, this will make :hover smoother.

  I hope this was helpful 👨🏻‍💻 overall, you did a great job, well done. Cheers 👾

  Marked as helpful

  1

  ---

  

  Chris•250

  @chintriagoPosted about 3 years ago

  @kens-visuals thank you for your feedback! I definitely am going to do some reading up on accessibility. Also I completely forgot about hover and transition thanks!

  0