Responsive Interactive Rating Component

Community feedback

• 

  Elaine•11,400

  @elaineleungPosted over 2 years ago

  Hi Jacob, about your question on improving the JS, firstly, I think you've done well on the whole! 🙂 The variables are clearly named, and everything is easy to follow.

  The main suggestion I have is, I would definitely change your var variable to let, and I also suggest not using var when you are using const and let. Since ES6, it's more common to see const and let being used instead of var, and in fact I think both of them are seen as replacements for var and are superior because of their clear separation of concerns (namely one is fixed, one can be reassigned), and in certain cases using var could have scoping issues.

  The other suggestion I have is, since you're using the disabled attribute for your button, you can just write that directly into the HTML of your button, like this:

  <button type="submit" disabled>Submit</button>
  

  That way, you don't need to write a line in your JS to change the state when initializing.

  Lastly, I see you using the :focus pseudo selector for your buttons, which I also saw someone else doing. The only issue with that is, if I click on something else that's not another button, the selected button would lose its focus and become deselected even if I didn't select another button. I'd suggest using classes instead to make sure the style would remain on the button. I would also add a cursor: pointer the all the clickable elements so that users know that can be interacted with.

  Great work completing your second challenge, and happy coding! 😊

  Marked as helpful

  2

  ---

  

  Jacob Marshall•270

  @JacobMarshall0Posted over 2 years ago

  @elaineleung Thank you, this feedback is great! I didn't realise that would be an issue with the :focus so thank you very much, I'll keep this in mind for future implementations.

  1
• 

  raionus•310

  @sinjin25Posted over 2 years ago

  I think that this piece of code is pretty fragile: rating=ratingButtons[i].innerHTML because if someone changed the html structure it would fall apart.

  You could try textContent or even more reliable would be to use an arbitrary attribute (I used data-value="3") to store it, that way the value is independent of the labeling.

  For your event handling. I think you're using the old style of event listeners (not sure). I think the most commonly accepted way right now is using event listeners. So someEle.addEventListener('click', () => { /* do something */ })

  Marked as helpful

  2

  ---

  

  Elaine•11,400

  @elaineleungPosted over 2 years ago

  @sinjin25 I agree about not using innerHTML. In this case, since buttons are used, they have an value attribute already that can be used. The data-value attribute is more for other elements that don't have a value attribute (such as divs and non-input elements).

  Also, yes, event listeners are far more common nowadays especially when writing vanilla JS (and also jQuery). I think the only times I use onclick is in JSX format when building with React.

  0

  

  Santiago Morales•370

  @AleromsPosted over 2 years ago

  @elaineleung Hi, is it appropriate to use innerHTML to edit the <p> to insert "you have chosen " + userinput + " of 5"?

  0

  

  Elaine•11,400

  @elaineleungPosted over 2 years ago

  @Aleroms Hi Santiago, I think it's OK for something like this because the text is already predefined by you and there's nothing created by the user using a text input, hence less of a security risk. I would prefer textContent if it's just plain text (which I think you can try first), but in any case I also used innerHTML in my solution. For an actual website in production, I don't think I'd be using it though, as I'd be more likely to use a framework to write everything.

  1