Site using CSS Grid

Community feedback

• 

  Raymart Pamplona•16,100

  @pikapikamartPosted about 3 years ago

  Hey, great work on this one. Layout in desktop seems fine as well for mobile layout. The functionality works but I don't know why I couldn't select the percentages, I could select them but only after adding the bill and the #of people.

  Some suggestions would be:

  • On the body tag, using height: 100vh is not great since it will limit the elements height based on the remaining viewport's height. Replacing it with min-height: 100vh is much better, because this will expand if it needs to.
  • Remove the width: 100vw on the body as well since this creates a horizontal scrollbar which you can see. block elements like body have a default of taking the 100% of the screen's width, so you don't need to specify it.
  • The alt value of the img inside the header could only used alt="spliter" only. Avoid adding words that relates to "graphic" like "image, icon, illustration" on the alt attribute.
  • On your h1 I see that you are making the text be for screen-reader only, but right now it is not being used that way. Using display: none only just completely remove it in the dom, you can search for sr-only class on google and apply those :>
  • The visual indicator for the button could have been better, right now it is hard to know where I am at when using my keyboard. Maybe applying more outline-offset would be better.
  • On the tip percentage, I would not use button for those, instead if I were to tackle this, I would use a fieldset and radio buttons, since those are selections and I can select only 1, and radio buttons are intended for those.
  • It would be great to have a aria-live element, the live element will say what button has the user pressed, because using just button there is no indication that is has been selected, that is why creating a aria-live element would be really great.
  • I would as well wrap the tip amount and total in a heading tag, the reason is that, if I navigate through that heading, I will know that the next element could either be the tip amount value or the total value.
  • For the reset functionality, it would be great as well to have an aria-live element, announcing that the calculator has reset its value.

  Other than those, great work on this one. I haven't checked your js but i'm leaving the refactoring to you :>

  Marked as helpful

  0