Tip Calculator Using HTML, CSS and Javascript

Community feedback

• 

  Raymart Pamplona•16,100

  @pikapikamartPosted about 3 years ago

  Hey, great work on this one. Desktop layout looks great, just a little bit shorter than the design. Site is responsive and the mobile state looks great as well.

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

  • Since you are using 2 container for the logo and the form itself, I suggest using:

  <header /> # contains the logo
  <main /> # contains the main component
  

  This markup will let you have landmark wrapping each of the content.

  • Remember that a website-logo is one of the meaningful images on a site so use proper alt for it. Use the website's name as the value like alt="splitter".
  • When using img tag, you don't need to add words that relates to "graphic" such as "logo" and others, since img is already an image so no need to describe it as one.
  • It would be great to use a form tag to wrap the main component in here since there are data that will be controlled by the user.
  • Those text like the bill and number of people could have been used as the label for each of the input that is below them. It would be great to use that so that the input will have a labelling text describing the purpose and the info needed for each.
  • Currently, those error messages are only seen visually but not really linked with their respective input tag. A pseudocode of a proper error showing would be like this:

  if ( input is wrong )
    input.setAttribute("aria-invalid", "true");
    input.setAttribute("aria-describedBy", id of the error-message);
  else 
    input.removeAttribute("aria-invalid");
    input.removeAttribute("aria-describedBy");
  

  The error-message element should have an id attribute which is referenced by the aria-describedBy attribute on the input element. By doing that, your user will know that the input is wrong because of aria-invalid and they will know what kind of error they made because of the aria-describedBy.

  • Also, the error-message is still seen by the browser since you are using only a opacity styling. When you are hiding an element, adding either visibility: hidden or display: none on their hidden state is the proper way so that they will be totally hidden for users.
  • For the tip selection a preferred markup on that one would be to use a fieldset tag with a legend tag as well. The legend text content will be the select tip word. You will have to use 5 radio buttons with their respective label tag:

  <fieldset>
    <legend>Select Tip</legend>
    <div>
      <label for="5%">5%</label>
      <input type="radio" name="5%" value="5%" id="5%">
    </div>
    ......
  </fieldset>
  

  It is preferred because if you think about it, you can only select a single tip percentage right and in a set of radio buttons, you can only select one as well. Using this markup as well will be much clearer for screen-reader users since it will always announce what is selected when user is traversing on the different selections.

  • The custom input needs to have a label on it, though it would be a screen-reader only label or you can use aria-label on the input as well. The text should describe what is the needed info for the input.
  • You can use heading tag on the tip amount and total amount since they give information on what the section would contain, hence the amounts from the calculator. Also, for this, it would be great if you somehow managed to use aria-live or output for the pricings so that whenever the user is finished doing the calculator, screen-reader will announce the calculated price right away for them. This way, they won't have to navigate and search on what is the result.
  • Also, the .attribution is getting in the way when opening up dev tools at the bottom. I was going to suggest using a flebox with flex-direction: column on the main container but I saw that your markup looks like this:

  <div /> # left side of the calculator
  <div /> # right side of the calculator
  <div /> # attribution
  

  If you somehow managed to nest the two part of the calculator in a single container, that would be great so that you can remove the position: absolute on the .attribution and you can just use the flexbox.

  Aside from those, great job again on this one.

  Marked as helpful

  1

  ---

  

  Soumya Banerjee•425

  @soumya495Posted about 3 years ago

  @pikamart Thank you for such an in-depth review, there is so much of new stuff that I got to learn from this. I will make sure that I'll apply these when I am building my next project.

  1
• 

  Marta•630

  @martam90Posted about 3 years ago

  Hi, I would have a look on those issues:

  1. I can't see the number of people in the input. I choose the number, but it won't display there.
  2. In JS I would add/remove classList instead of hard coding css styles.
  3. It seems to me that total amount and tip amount are calculated wrong. There is one more zero add to every amount. The tip for the bill of 100$ for 1 people should be 10$, instead I see 100$.

  Overall, I think you have done nice work :)

  Marked as helpful

  1
• 

  Soumya Banerjee•425

  @soumya495Posted about 3 years ago

  Thank You Marta for bringing this to my attention, actually the problem was not with the logic but the digits of input were hiding under the element that I positioned absolutely in the input field to hide the arrow keys of number field, I was not aware of the fact that it could be done from CSS, so now I've changed the code and the issue seems to be fixed, If you can please do check out. Thank You again for the review .

  0

  ---

  

  Marta•630

  @martam90Posted about 3 years ago

  @soumya495 Hi, that's ok, I see you have solved this issue. Now, it works very well. Good work! :)

  1