Interactive rating component

Community feedback

• 

  P

  Grog the Frog•480

  @GregLyonsPosted over 2 years ago

  Everything works well, and your design matches, so good job there. I have a few suggestions for your code.

  1) I mentioned in a comment on another one of your solutions that should be using semantic HTML (for accessibility and SEO reasons). This is a case where it becomes pretty important: your rating numbers and submission button should be <button> elements instead of <div>'s. Even though someone using a mouse wouldn't tell the difference, it's not possible to use your form with a keyboard. This is because I'm not able to focus on the interactive elements of your page using the "Tab" key. Some people need to use keyboards rather than a computer mouse to navigate the web, whereas others just prefer to use them on forms (like when filling out credit card info on a to-go app, I like to tab between the fields instead of clicking).

  Another nice thing about using semantic HTML (e.g. <button>'s instead of <div>'s), is that you don't actually need to code any support for tabbing. On the one hand, you could use JavaScript to allow the user to tab between your <div>'s, but that would be quite complicated. Instead, if you use <button> elements, that behavior is already built-in. Thus, you're able to make your web page a lot more interactive without much more effort.

  2) You have a couple resets at the top of your CSS file:

  *, :active, :focus {
      outline: none!important;
  }
  *, :after, :before {
      box-sizing: border-box;
  }
  

  The second one, the box-sizing: border-box;, is one I use all the time. You should be careful with the first one that removes the outline on button elements, however. For one, I don't believe it actually applies to your solution, since you aren't using elements which can be focused (and thus have the :focus pseudo-selector). More importantly, it can make your website harder to use. Were you to use <button> elements like I suggest in my first point, then this reset would mean that users couldn't actually see (or at least would have trouble seeing) which button which button was selected, because the outline is what indicates which element is focused.

  I myself like to apply custom styling to my <input>'s and <button>'s, and sometimes prefer to turn the outline off. However, it's very important then that you add some sort of replacement styling. E.g. in your

  .submit:hover {
      color: var(--Orange);
      background-color: #fff;
  }
  

  rule, you could also add a .submit:focus selector:

  .submit:hover,
  .submit:focus {
      color: var(--Orange);
      background-color: #fff;
  }
  

  which could be a suitable replacement. Sometimes, if you're adding a :hover effect to a button, adding a :focus selector onto the rule can be an acceptable replacement to removing the outline, but not always.

  3) Your JavaScript, while perfectly functional, has a lot of repetition. Here are some tools you could use to cut down on it:

  • You could use document.querySelectorAll(".circle-icon") to select all your numbers at once, and store them in an array. I believe they should be stored in 1, 2, 3, 4, 5 order, then, because that's how their order is in your HTML. If not, you should be able to use their .id attribute to see which one is which.
  • Once you have your rating buttons in an array, you could use a for-loop to iterate through them an apply your event listeners. You can see that your event listeners look very similar, except with a couple numbers switched around. In pseudo-code (with ratingButtons your array from the previous point), it should be something like

  # `i` is the value of the rating button
  function onClick(i)
    for (ratingButton in ratingButtons)
      if ratingButton.id == i
        activate ratingButton
      else
        deactivate ratingButton
      
      set choice to i
  

  You could even use a for loop to assign this event listener to each rating button (that's why onClick takes the value i as an argument.

  Hope this helps!

  Marked as helpful

  0

  ---

  

  Abubakr Hisham•320

  @abubakr404Posted over 2 years ago

  @GregLyons Thanks again. I didn't think that I could put the elements in an array. I tried to access the elements directly, but I couldn't. I will focus on your advice.

  1