Order Summary component

Community feedback

• 

  Vanza Setia•27,775

  @vanzasetiaPosted almost 3 years ago

  👋 Hi Vincent Ferraro!

  Regarding your question:

  • Using px is not accessible at all since it is an absolute unit. Using px is obviously a bad practice it won't allow the user to scale the size of any elements on the page. So, I would recommend using rem unit most of the time. rem unit is a relative unit, which means that it can adapt through different scaling.
  • For the percentage unit, it is relative or depends on the situation. But most of the time you will use rem or em unit instead of %.

  Feedback:

  • 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 (Tab) easily
    • For any decorative images, each img tag should have empty alt="" and aria-hidden="true" attributes to make all web assistive technologies such as screen reader ignore those images. In this case, all images are decorative only.
    • Every page should have one h1 as an identifier. In this case, you can increase the heading level of Order Summary.
    • All page content should live inside a landmark (header, main, footer). In this case, you can wrap all page content with main tag, except your attribution. For your attribution, you can swap the div with footer tag.
  • Important
    • You should always include box-sizing: border-box as your common reset styling. This property will solve a lot of problems! See the demo on CSS Tricks.
  • Visual
    • On mobile view (360px * 640px), the card having full width. Prevent this by adding padding to body element.
    • On mobile landscape mode, the card gets cut off. The problem is you're limiting the height of the body element, it will not allow the users to scroll the page if the page content needs more height. Use min-height instead.
    • You're only using the desktop pattern image for the body background. You should use the mobile version for mobile and change the pattern image by using @media query.
    • I would recommend applying these styles to your body element.

  body {
      font-family: 'Red Hat Display', sans-serif;
  
      /* Remove this property since by default body element has 100% */
      width: 100%; 
  
      min-height: 100vh;
      display: flex;
      flex-direction: column;
      justify-content: center;
      align-items: center;
      background-image: url(/images/pattern-background-mobile.svg);
      background-size: 100%;
      background-repeat: no-repeat;
      background-color: <blueish-color>;
  }
  
  @media (min-width: <value>) {
    body {
       background-image: url(/images/pattern-background-desktop.svg);
    }
  }
  

  That's it! Hopefully, this is helpful!

  3

  ---

  

  Vincent Ferraro•20

  @vincentferraroPosted almost 3 years ago

  @vanzasetia Hello Vanzia, thank you very much for your amazing comments and your help. Most valuable for me. I'm going to take you advice and revise my files. :)

  0
• 

  Chris•240

  @Chris94LeePosted almost 3 years ago

  Hey Vincent, looking good!

  One thing I noticed is that you're missing a background colour. What I'd do is add background-color: #E0E8FF to your body in the CSS file. Also, to make the background image fit like the designs, you can add background-repeat: repeat-x; and then remove the background-size style you have on the body. Let me know if this works for you!

  Also regarding px for size, I try to avoid pixels for accessibility reasons, and tend to stick to either em or rem. Check out: https://engageinteractive.co.uk/blog/em-vs-rem-vs-px

  Looking forward to seeing your future work!

  1

  ---

  

  Vincent Ferraro•20

  @vincentferraroPosted almost 3 years ago

  @Chris94Lee Hello Chris, Wohaa Thank you for your advices! I'm going to read your article. :)

  0