Submitted over 1 year ago

Interactive rating component

backpackoder• 80

@MrFougasse

Preview site View code

This is a solution for...

HTML
CSS
JS

1newbie

View challenge

Design comparison

SolutionDesign

Solution retrospective

Hey there, but I am pretty noob with the DOM so I typed a looot of lines of code and I am sure I could have done this much smaller concise and understandable if I would know how to create only one function for all the 5 notes instead of create one function for each. So if someone is good enough to explain me, I take it! Everything is working well tho, so don't forget to click on the "5" and submit it :p

Community feedback

Denis• 1,060

@Mod8124

Posted over 1 year ago

Hello, well done!

You can reduce the code of your function by passing parameters and always put all your const & let on the top of the file in that way the variable it's the global scope and not the local scope(only available in the function or {}) also it's easy if you select all your buttons with querySelectorAll

  //select all the buttons 
 const buttons = document.querySelectorAll('.circleNbr');
 const outputNote = document.querySelector('#outputNote');
 
//then create a function, first, it's going to put all the buttons by default(background & color), and then it's going to change the background & color of what button that user clicked and change the outputNote value 

function noteSelected(event) {

  buttons.forEach(button => {
    button.style.backgroundColor = "#292D38";
    button.style.color = "hsl(217, 12%, 63%)";
});

   event.target.style.backgroundColor = "hsl(217, 12%, 63%)";
   event.target.style.color = "#fff";
   outputNote.innerHTML =  event.target.innerHTML;
  btnInHover.classList.add("btnInHover");
 
  }

//then assing your function `noteSelected` for every button by method forEach

buttons.forEach(button => button.addEventListener('click', noteSelected);

//now every button has an click event

and that's all  :)

the function submit is ok, also it's bad practice to call functions in your html, try to always assign event or call directly in your script tag

bad

 <button onClick="noteSelected()"></button>

good

 const button = document.querySelector('button');
 button.addEventListener('click', noteSelected);

Marked as helpful

backpackoder• 80

@MrFougasse

Posted over 1 year ago

@Mod8124 Thank you so much, your help blowed my mind!

Elaine• 11,420

@elaineleung

Posted over 1 year ago

Hi Harderth, well done on completing this! And yes, even though there's a lot of repetitive code, I think it's more important firstly that you're able to make it work and that you understand the principles that make it work. Now that everything's working, we can see how to make this better 🙂

I think Denis gave you some good advice on how to rewrite the code; I'll use some of the things he mentioned and provide additional adivce:

First, I see that you're using <p> for your buttons; I highly suggest that you use <button> instead because even though you can use <p>, it actually is not meant for this kind of task, and button is far more suited. With HTML, you always want to use the most appropriate element for the thing you want to do, and so my first recommendation is to change the five p tags to a button tag.
I see that you're using inline styles here in your JS. While this is doable, I recommend using a CSS class instead because, let's say you decide you want a different color for your button text and background. You'll have to make the change in your CSS and also in your JS, and sometimes when copying color values, it's easy to get them wrong. In any case, it's always best to keep the CSS out of the JS whenever possible. You can try having a selected class that only has the styles of the selected button, and then in the JS, just add the class when the button is selected and remove the class on the other buttons:
```
// CSS

.selected {
  background-color: hsl(217, 12%, 63%);
  color: #fff;
}

// JS 

function noteSelected() {
   notes.forEach((note) => {
    note.classList.remove("selected");

    if (note.textContent === outputNote.textContent) {
      note.classList.add("selected");
    }
  });
}
```
You got some accessibility issues in the report, so you might want to look into those. The easiest one to tackle there is the one where you need a main tag; simply wrap a <main> tag around all your content (basically everything inside <body>), and that should do it.
Lastly, I see that you got your CSS and JS in your HTML file; it's generally best to keep them separate, so for your next challenge, you can try having a separate CSS file and a separate JS file (if you need JS, that is).

I put together a CodePen that shows the things I mentioned here, and you can check it out and play with it! CodePen here: https://codepen.io/elaineleung/pen/RwMmgmw

Hope this helps you, and do keep going! 😊

Marked as helpful

backpackoder• 80

@MrFougasse

Posted over 1 year ago

@elaineleung Thank you so much, your help blowed my mind! 4 : I will.

Please log in to post a comment

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