0

I am making a card like this: enter image description here

The expected behaviour is that when I click any of the rating circles, their color should change to grey for which I have a class defined in my css that I toggle using js code.

The codepen for my solution is this. Using the js code that I have written, whenever I click any of the ratings, the last one turns grey all the time.

This is my current code:

// getting the objects
const submitBtn = document.querySelector('button');
const ratingsParent = document.querySelector('.ratings');

console.log(ratingsParent.children);

// sanity check
submitBtn.addEventListener('click', () => {
    console.log('Clicked');
})

for (var r of ratingsParent.children) {
    r.addEventListener('click', ()=> {
        r.classList.toggle('clicked');
    })
}

But when I change r.ClassList.toggle to evt.target.ClassList.toggle, it shows the expected behaviour. Im extremely new to JS and just starting to learn frontend dev so Im unable to figure out the reason for this behaviour.

Sunderam Dubey
  • 1
  • 11
  • 20
  • 40
Aman Savaria
  • 164
  • 1
  • 1
  • 11

1 Answers1

2

var has different behaviour than let and const when declared in a loop header – it only creates a single variable for the entire loop, which all closures share. You clearly have support for let and const, so you should literally never use var. (Maybe there’s a linting rule you can enable?)

for (const r of ratingsParent.children) {
    r.addEventListener('click', ()=> {
        r.classList.toggle('clicked');
    });
}

There’s also the option of attaching a single listener to the parent and letting events bubble up, note.

ratingsParent.addEventListener('click', e => {
    if (e.target.parentNode === ratingsParent) {
        e.target.classList.toggle('clicked');
    }
});
Ry-
  • 218,210
  • 55
  • 464
  • 476
  • 1
    You should use `document.querySelectorAll(".ratings")` to get all the elements containig this particualr class. Then you can simply loop through the array with a `forEach` loop like this: ``` ratingsParent.forEach((elem) => { elem.addEventListener("click", (e) =>{ elem.classList.toggle("clicked"); }); }) ``` – Ali Mustafa Aug 20 '22 at 07:25
  • 1
    @AmanSavaria: Although actually, you should use `` for a lot of built-in accessibility (keyboard support, screen reader support, etc.) – see https://codepen.io/rnoh/pen/YzaBazK (still needs focus states though) – Ry- Aug 20 '22 at 07:33
  • Thanks a lot for the comment Ry, Im very new to the world of frontend and have started just a few weeks ago so I didn't know this. Thanks. – Aman Savaria Aug 20 '22 at 16:44
  • Thanks Ali. Yeah I thought of that approach but before that I wanted to know whats conceptually wrong with my approach. – Aman Savaria Aug 20 '22 at 16:44