Interactive Rating Component

Solution retrospective
My first project with Javascript. I would really appreciate any suggestions for improvement. I learnt Bem Sass and Javascript by building this component.
Please log in to post a comment
Log in with GitHubCommunity feedback
- @Cats-n-coffee
Hi Vinzz!
Great job for a first time using Js! I think you picked a good project to start with the language. This project is pretty small, so I'll try to give feedback on everything I see:
- as a general rule, you want to give all your variables a real human readable and meaningful name, so avoid
x
but maybe use something likebutton
. Maybe the only accepted exception isfor
loop and thelet i = 0
(or similar if you're doing a reverse loop) wherei
is known to be short forindex
. Good variable names will help you in bigger projects, and help yourself and others understand your code with the least ambiguity possible. You'll find that good variable names are a real mental exercise :) undefined
is a falsey value in Js (among other values), line 18 can probably beif (value)
because it'll evaluate totrue
once it has a value. This is a good page to read https://developer.mozilla.org/en-US/docs/Glossary/Falsy- I'll just give my 2 cents on the logic/UX. You're allowing to send a 0 rating, but it's not in the choice list. The user doesn't know that submitting without selecting a number gives a zero rating. They could also accidentally click the submit button without having anything selected. I would suggest to add 0 as an option, or disable the submit button it there is nothing selected.
- since you said it's your first Js project, I'll point you to another way of looping through elements, where you use
querySelectorAll
andforEach
(this is my recent submission https://github.com/Cats-n-coffee/FEM-timeTrackingDashboard/blob/master/src/index.js#L136). You'll this used often. - be careful with
innerHTML
has it can lead to security vulnerabilities if you are not fully aware of what you're using it for. You seem to be using/retrieving only text and no HTML, so you could useinnerText
ortextContent
. There is a difference between the 2 I'm suggesting, but I'll let you read on that. - Great job using
const
andlet
and thinking through this project!
Hope this helps!
Marked as helpful - as a general rule, you want to give all your variables a real human readable and meaningful name, so avoid
Join our Discord community
Join thousands of Frontend Mentor community members taking the challenges, sharing resources, helping each other, and chatting about all things front-end!
Join our Discord