Skip to content
OverviewSolutions
12
Comments
2
Paweł
@H4m4k

All comments

  • Paweł•240
    @H4m4kPosted 9 months ago

    Hi Onos Ejoor! Congrats on solving the challenge - good job!

    • I have noticed that you are using <section> tag for all of your markup. It works but i would point your attention towards reading HTML 5 Semantic elements.
    • consider using background-repeat: no-repeat in your header ID
    • the design is supposed to be created to a max width of 1440px , so I would advise creating a class ( e.g. container or wrapper ) in your CSS and adding max width as described in the style guide - like so : .container { max-width: 1440px }
    • after that i would add margin: 0 auto; - .container { max-width: 1440px ; margin: 0 auto;}
    • margin: 0 auto - will set automatic margin on left and right after limiting the width and in effect center your content
    • after we have our width set and content centered we can add this class to each <section> in body you have or if you read the article i've posted , add class to <header> , <main> and <footer> tags.
    • in case You need a source of tutorials check freeCodeCamp
    • last tip i would suggest using W3C Validator before submitting your solution, you will catch all the possible errors before we catch them :)
    Marked as helpful
    1
  • Martha•510
    @Mtc-21Submitted about 3 years ago

    I tried to make this challenge as accessible as possible, I had some inconveniences, because I wanted to make the results automatic as the data was entered, I avoided as much as possible the excessive use of html tags, making use of grid, your comments and suggestions are welcome.

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

    2
    Paweł•240
    @H4m4kPosted about 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