responsive design

Community feedback

• 

  Vanza Setia•27,775

  @vanzasetiaPosted almost 3 years ago

  👋 Hi Belkassem!

  🎉 Congratulations on finishing this challenge! I have some feedback that will improve this solution:

  • Accessibility
    • Create a custom :focus-visible styling to any interactive elements (button, links, input, textarea). This will make the users can navigate this website using keyboard (Tabs) easily.
    • Each img should have alt attribute, but since all images are decorative images, you can add role="presentation". By adding role="presentation", you are allowed to not have alt attribute.
    • Keep in mind that the above method hasn't get widely supported. So, the best approach is adding the empty alt="" attribute and aria-hidden="true". To learn more about how to handle decorative image, you can read the W3 Accessibility Tutorials.
    • All page content should live inside a landmark (header, footer, main). It's important since screen reader or other assistive technology uses landmark to navigate and understand the page content. You can wrap all the page content with main element (except the attribution). For the attribution you can swap the div with footer.
    • Only have one h1 for every page, I would recommend changing all h1 to h2. This will generate an issue where it says "every page should have one heading one" (something like that), so you can fix the issue by adding "hidden" h1.

  <main>
     <h1 class="sr-only>3-column preview card component</h1>
     // sr-only = screen reader only (hidden visually)
  ...
  

  • Use rem or sometimes em unit instead of px. Using px will not allow the users to control the size of the page based on their needs.
  • Responsiveness
    • On my mobile view (360px * 640px), I have a horizontal scrollbar and on desktop view (1280px width) does have the same problem. This issue is coming from the parent-container div, which has a fixed width and height.
    • I would recommend making the body element as the main container in this, since it's possible and you don't need to set any width and height to the body element. This will make sure that the body element can "shrink and grow" (responsive).
    • Use flexbox to make those cards perfectly on the center.

  // This is what I did
  // Depending on your markup, yours might be different
  // 👇 For desktop
  
  1. Making sure that the body element has full height, but
      still allow it to grow and  make the card centered vertically
  body {
    display: flex;
    align-items: center;
    justify-content: center;
    min-height: 100vh; /* 1 */
  }
  

  • Other
    • The headings don't have the correct font family based on the design comparsion.
    • I think the Learn More buttons should be links since they usually will navigate the user to another page. But, if you have a reason for using button element, then I would recommend adding type="button" to prevent the browser from submitting any information.

  That's it! Hopefully, this is helpful!

  Marked as helpful

  1

  ---

  

  belkassem mustapha•110

  @developython14Posted almost 3 years ago

  @vanzasetia thank you very much it was a great explanation and details to make the solution more professional thank you again

  0