interactive-rating-component

Community feedback

• 

  P

  Grog the Frog•480

  @GregLyonsPosted over 2 years ago

  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

  0

  ---

  

  Eugi•270

  @EugiSsPosted over 2 years ago

  @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 :)

  1
• 

  Kamasah-Dickson•5,590

  @Kamasah-DicksonPosted over 2 years ago

  I really like your solution and also the rotation you added. But your card is not responsive on smaller devices, there is this overflow.

  So I suggest you use max width on your cards instead of just width.

  Besides good job there👍 Happy coding👍

  0

  ---

  

  Eugi•270

  @EugiSsPosted over 2 years ago

  @Kamasah-Dickson Yes, I really didn't notice that the card drops out on smaller devices. I checked the functionality and forgot about it. Thanks for your comment and rating!

  0

  

  Kamasah-Dickson•5,590

  @Kamasah-DicksonPosted over 2 years ago

  @EvgiSs you are always welcome👍

  0