Tip calculator app using html, css js, grid and flexbox

Community feedback

• 

  Paweł•240

  @H4m4kPosted almost 3 years ago

  Good Job, congratulations on finishing the challenge.

  Some points I would change :

  HTML/ CSS

  • class result has display grid on it, Bill Sum per person text does not fit inside the grid because of the size of grid lanes, try changing the size of the lanes. You have used grid-template: 1fr 1fr , which you can easily adapt to a more fitting sizes and prevent going out of bounds on some resolutions. .4fr and .6fr will do a much better job, because You split 1fr unit, which is much easier to understand and covers the whole class and split it into 40% of the fraction and 60% of the same fraction, which makes your Bill Sum
  • border solid 1px in my opinion should be minimum 2px or .1em to instantly see the red
  • as a rule of a thumb ,check your HTML in W3C validator and solve problems before posting :)

  JS

  • entering small bill like 5$ and 6 people makes your calculation display that it can't be greater than bill, I advise to revise your logic as its a extreme case but still within boundries of normal operation
  • const removeSelect with anonymous function and a for loop to remove the class is a very hardcore way of solving an easy problem, just use a document.querySelector('.button.select').classList.remove('select') to remove the selection and preserve resources
  • also I have noticed You are using a non strict comparison operator == which in JS can lead to strange behaviour, so I would advise against using that and use === strict comparison
  • lastly You are using another for loop in a simple task of looping through elements, check the porcentage[i].addEventListener part - line 58. Instead please have a read about a high order functions - forEach is the one You should use instead of a for loop.

  Cheers Martha

  Marked as helpful

  0

  ---

  

  Martha•510

  @Mtc-21Posted almost 3 years ago

  @H4m4k thanks for your comments, I use a validator html and css, as well as the WAVE tool helps me a lot, I corrected some parts, but other details came out that I will correct on the fly, again thanks for your comments

  0
• 

  Alex Manzo•20

  @alexmanzoPosted almost 3 years ago

  Nice work overall! You did a great job recreating the design.

  I didn't dig into the actual code, but a couple notes I have just previewing/inspecting/using:

  • I have one tip on using borders for hover/focus states. Right now you'll notice when you hover/focus, the addition of the border resizes the input slightly. One trick to avoid that awkward resize is to set the default style to 1px solid transparent.

  • The "Can't be negative" feedback immediately popping up when you first try to change the value is a little confusing. Ideally, that feedback would appear on blur if I've failed to enter a positive number.

  • Another thing I would also advise is to avoid using background images when that image contains text. It would be great here to have an <h1> with "Splitter" as the text - even if it's screen reader only! Or alternatively, using an actual <img> element with alt text.

  Marked as helpful

  0

  ---

  

  Martha•510

  @Mtc-21Posted almost 3 years ago

  @alexmanzo thank you very much, it didn't work the use of 1px solid transparent, I will try again later

  0