@ArhyelTarfaSubmitted almost 2 years ago
It was super difficult for me, i was not able to perfect it but i learnt a lot from it.
Gabriel Bezerra
@GabrielBezerraGAll comments
- @GabrielBezerraGPosted almost 2 years ago
Hey Arhyel, I went through your code and I'd like to share some things that could be improved:
- You could style the ".container" div to center the content on the screen. Apply this to the desktop layout using a media query.
- Change the phone input type to 'tel'
- The plan buttons should be inputs type 'radio' and have the same 'name' attribute so that only one option can be selected. You could do the same thing you did with the monthly switch where you kept the input hidden. In this case, you would have to verify which one is checked to apply the active style and use it in the finishing up step
- I noticed there's something wrong with the 'addon' event listener. When the input is clicked, the event fires twice. The preventDefault() method seems to solve it but the input is not always selected in the event depending on where you click, which can cause an error. I'd suggest not to use event.target and choose a different approach.
- The add-on inputs must have the same 'name' attribute
- Instead of using .children and .childNodes you could use a querySelector in your parent element to prevent unexpected errors when you change the structure of your code
- Instead of the 'add' array, I'd verify which add-ons are checked on the submit event. This array is never cleared and it keeps adding elements when an input is clicked
- Consider breaking your code into smaller pieces using different CSS files and js modules to keep it more organized
Overall, I think you did a nice job and I hope this helps you finish your project!
1
