Responsive Layout using CSS Grid

Community feedback

• 

  Raymart Pamplona•16,100

  @pikapikamartPosted about 3 years ago

  Hey, awesome work on this one. Layout in desktop looks great though I had to zoom out since you are using 1440x as the desktop breakpoint which is too large for my screen and for lots of users who uses a traditional 1366x768 screen. The 1440px on the design does not mean it is the breakpoint, no, so lowering it down would be really great. Mobile layout looks great.

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

  • It would be great to have a base styling with these properties

  html {
    box-sizing: border-box;
    font-size: 100%;
  }
  *,
  *::before,
  *::after {
    box-sizing: inherit;
  }
  

  This way, handling an element's size will be lot easier because of the box-sizing: border-box. You can search up some more of how this works.

  • 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.
  • Those car icons are just decorations. 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 "illustrated or illustration" and others. An img is already an image/graphic so no need to describe it as one.
  • The learn more is better using a tag since it is more likely to be a link on a real site to "learn more" about the specific car.

  Aside from those, great job again on this one.

  Marked as helpful

  1

  ---

  

  Matt Cowan•40

  @mattbcowanPosted about 3 years ago

  @pikamart Thanks for the feedback! I've updated the portions for view height and the html box sizing as well as the aria-hidden's I missed. Appreciate those suggestions.

  Question, for the learn more you suggested a tags rather than buttons but wouldn't the buttons be more accessibility friendly?

  1

  

  Raymart Pamplona•16,100

  @pikapikamartPosted about 3 years ago

  @mattbcowan It will be the same accessibility friendly since both are interactive elements. However, that "learn more" will be more likely an own page of the site showing information on a selected car and since it is a page, you will need to use a link a tag and not button.

  Because button are used for controlling something, maybe like showing a popup, submitting a form, closing a form, a dropdown and much more.

  1

  

  Matt Cowan•40

  @mattbcowanPosted about 3 years ago

  @pikamart Gotcha. I'll need to look into it more but what I'm understanding is a button is for controlling something where as an a tag is for linking to something and even if in the mockup it looks like a button if it's just linking to a page, external website, etc. I should be using an a tag.

  1
• 

  P

  Ken•4,915

  @kens-visualsPosted about 3 years ago

  Hey @mattbcowan 👋🏻

  I have some quick tips to help you fix the accessibility issue.

  • Instead of an empty main, <div class="card">...</div> should be <main class="card">...</main> and this should fix the accessibility issues. Don't forget to generate a new repot once you fix the issues.
  • For the car icons, add aria-hidden="true”, because they are for decoration.
  • Also, I suggest adding transition: all 0.2s; to the button and the links, this will make :hover smoother.

  I hope this was helpful 👨🏻‍💻 all in all, you did a great job, especially with responsiveness, well done. Cheers 👾

  Marked as helpful

  1

  ---

  

  Matt Cowan•40

  @mattbcowanPosted about 3 years ago

  Hey @kens-visuals

  Thanks again for the suggestions. I went ahead and made those adjustments. The aria-hidden's are still giving me trouble but I'm starting to catch them in my workflow.

  As always, appreciate the feedback!

  0

  

  P

  Ken•4,915

  @kens-visualsPosted about 3 years ago

  @mattbcowan you're welcome 😇

  0