3-column-preview-card-component

Community feedback

• 

  Raymart Pamplona•16,100

  @pikapikamartPosted almost 3 years ago

  Hey, great work on this one. Desktop layout looks fine, it is responsive though the upper part when going into like tablet size is being hidden (suspecting vh in here), mobile state is fine though.

  Rafael already gave feedback on this one, just going to add some suggestions as well:

  • Don't use width: 100vw since this will only add a horizontal scrollbar at the bottom, since this value does not account the vertical scrollbar's width.
  • Avoid using height: 100vh on a large container like the body as this makes the element's height capped based on the viewport/screen's height. Instead use min-height: 100vh so that the element will expand if it needs to.
  • Inside the main, why isolate the 3rd box and wrap the 1st and 2nd box inside of another div? You can just made use of:

  main
    >div
    >div
    >div
  

  • section in here is not needed and also, section as landmark is not informative unless it is labelled by aria-labelled by attribute. Changing it to using only div would be better.
  • Those car icons are only decorative. Decorative images should be hidden for screen-reader at all times by using alt="" and aria-hidden="true" to the img tag or only aria-hidden="true" if you are using svg instead of img tag.
  • Only use descriptive alt on images that are meaningful and adds content to the site otherwise hide the image for screen-reader users.
  • On a site, always have a single h1. Since there are no visible text that are suitable to be h1, the h1 would be a screen-reader only heading. Meaning it will be hidden visually but present for screen-reader users. On this, the h1 would have like sr-only class and the text-content should describe what is the main-content is all about. The h1 could be placed as the first text inside the main.Have a look at Grace's solution on this one inspect the layout and see the usage of h1 as well the stylings applied to it.
  • Using a tag would be better for the learn-more since on a real site, those would be page links where user can "learn" more about a specific car.

  Aside from those, great job again on this one.

  Marked as helpful

  1

  ---

  

  Doc•100

  @GodILoveTequilaPosted almost 3 years ago

  @pikapikamart Thanks a lot! This was super helpful, i really appreciate it! The reason why i added the top div and bottom div is so that i can make a media query for something in between phone and desktop size screens because it didn't liked the way the cards were shrinking in size.

  1
• 

  Rafael•200

  @StomperHKPosted almost 3 years ago

  Yes. Put this CSS declaration inside the grid container. grid-template-columns: repeat(auto-fill, minmax(250px, 1fr));

  Marked as helpful

  1

  ---

  

  Doc•100

  @GodILoveTequilaPosted almost 3 years ago

  @StomperHK Thanks for the answer. Witch one do you think will take less time to accomplish? And is one or the other a best practice in this card context?

  1

  

  Rafael•200

  @StomperHKPosted almost 3 years ago

  @GodILoveTequila In this case, it would be cool use Flexbox (at least for me), because you are controlling the items along the horizontal axis only, not both vertical and horizontal axis. I answered using grid-template-columns because it was the easiest way to answer your question... Unlike it would be if i used a flexbox code. But it's up to you.

  Marked as helpful

  1