@GregLyons
Posted
It looks like all of your HTML, CSS, and JavaScript is pretty solid. Nice!
The one thing I'd suggest changing would be to put the rating numbers in <button>
elements. Right now, the user can only fill out your form with a mouse, since they can't focus the ratings to select them. While you could make the <li>
elements themselves focusable, you'd have to worry about other things like making sure the Enter
key also selects the rating using another event listener, and so on. Instead, the <button>
element is focusable by default, and pressing Enter
activates the click
event, so it's a lot easier to work with. Doing this would make your website more accessible, not just for users who can't use a mouse, but also for users who prefer to use a mouse.
As I mentioned before your JavaScript is good, but since you said that you're learning to write JS, let me give you a tip that you may want to consider for future projects:
If, instead of passing an anonymous callback function into addEventListener
, you first define a function beforehand and then pass in that function to addEventListener
, it then makes it easier for you to remove the event listener in the future when necessary, like so:
const handler = function(e) {
...
};
document.addEventListener("...", handler);
...
document.removeEventListener("...", handler);
In other words, you can just pass in the name of the function to removeEventListener
. See Adam Heath's answer on this StackOverflow post for more details.
Obviously you don't need to do that in this challenge, but you'll likely find that you need to remove an event listener at some point in the future.
Marked as helpful
@EugiSs
Posted
@GregLyons You're right, using a <button> would be much better if you thought about it, thanks. I will definitely keep this in mind in the future. I have been researching about removing an event listener, but here I decided to write an anonymous function since nothing needs to be removed. But anyway, thanks for the reminder. And thanks for the rating and advice :)