JavaScript - Adding event listener to an array of objects

Question

I am new to the JavaScript Language so please help me understand where I am going wrong with this issue. I am trying to toggle the done class on and off when a list item is clicked. But I am getting this "Uncaught TypeError: Cannot read property 'classList' of undefined at HTMLLIElement" error.

var li = document.querySelectorAll("li");

for (var i = 0; i < li.length; i++) {
  li[i].addEventListener("click", function() {
    li[i].classList.toggle("done");
  })
}

.done {
  text-decoration: line-through;
}

<h1>Shopping List</h1>
<ul style="list-style-type:square">
  <li>Pencil</li>
  <li>Paper</li>
  <li>Eraser</li>
  <li>Table Lamp</li>
  <li>Comb</li>
</ul>

what if I add more li items to the ul by using appendChild() method? Any of the suggested answers doesn't seem to work with the newly added items. How to solve this issue? — sibasishm, Jul 02 '18 at 14:20

Guillaume Georges · Accepted Answer · 2018-07-02T16:45:18.373

When adding your event listener, you are creating a closure. Each event handler has the variable i in their scope. Unfortunately, it's the same variable i, the one that's incremented by your for loop.

At the end of the loop, each event handler has, in its scope, a variable i whose value is now 6. It results in li[i] being undefined when the event is fired.

You can solve this by, either :

not using i in your handler, for instance by using the provided event variable, that references the event. And thourh it, retrieves the targeted element :

var li = document.querySelectorAll("li");

for (var i = 0; i < li.length; i++) {
  li[i].addEventListener("click", function(event) {
    event.target.classList.toggle("done");
  })
}

.done {
  text-decoration: line-through;
}

<h1>Shopping List</h1>
<ul style="list-style-type:square">
    <li>Pencil</li>
    <li>Paper</li>
    <li>Eraser</li>
    <li>Table Lamp</li>
    <li>Comb</li>
</ul>

declaring i with let instead of var in your for loop. This will result in each of your closure to have a different binding with i :

var li = document.querySelectorAll("li");

for (let i = 0; i < li.length; i++) {
  li[i].addEventListener("click", function() {
    li[i].classList.toggle("done");
  })
}

.done {
  text-decoration: line-through;
}

<h1>Shopping List</h1>
<ul style="list-style-type:square">
    <li>Pencil</li>
    <li>Paper</li>
    <li>Eraser</li>
    <li>Table Lamp</li>
    <li>Comb</li>
</ul>

attaching the event listener to the UL. This would be my favorite :

var ul = document.querySelector("ul");

ul.addEventListener("click", function(event) {
    event.target.classList.toggle("done");
  }, true);

.done {
  text-decoration: line-through;
}

<h1>Shopping List</h1>
<ul style="list-style-type:square">
    <li>Pencil</li>
    <li>Paper</li>
    <li>Eraser</li>
    <li>Table Lamp</li>
    <li>Comb</li>
</ul>

Read more about JavaScript closures.

Thank you for such a detailed explanation. This is exactly what I was looking for. — sibasishm, Jul 02 '18 at 13:48

score 1 · Answer 2 · answered Jul 02 '18 at 10:40

1

Change

li[i].classList.toggle("done");

to

this.classList.toggle("done");

...otherwise in the event callback you'll always be targeting the li represented by the last value of i, not the one that was clicked.

answered Jul 02 '18 at 10:40

Mitya

33,629
9
60
107

score 1 · Answer 3 · answered Jul 02 '18 at 10:46

To improve the performance of your code, you can use event delegation. With that technique, you just need to add the listener once, to the container (in this case, the ul), and all the children will have access to it. Then, you can use the event.target property to target every li item:

var ul = document.querySelector("ul");
ul.addEventListener('click', function(event){
  event.target.classList.toggle("done");
});

score 0 · Answer 4 · edited Jul 02 '18 at 11:24

0

//using for of loop
var li = document.querySelectorAll("li");
for (var i of li) {
  i.addEventListener('click', function(e) {
    e.target.classList.toggle("done");
  })
}

.done {
  text-decoration: line-through;
}

<h1>Shopping List</h1>
<ul style="list-style-type:square">
  <li>Pencil</li>
  <li>Paper</li>
  <li>Eraser</li>
  <li>Table Lamp</li>
  <li>Comb</li>
</ul>

edited Jul 02 '18 at 11:24

Andrew Bone

7,092
2
18
33

answered Jul 02 '18 at 10:53

vicky patel

699
2
8
14

1

While this code snippet may solve the question, [including an explanation](http://meta.stackexchange.com/questions/114762/explaining-entirely-code-based-answers) really helps to improve the quality of your post. Remember that you are answering the question for readers in the future, and those people might not know the reasons for your code suggestion. – Andrew Bone Jul 02 '18 at 11:24
i appriciate your advise and i think about it – vicky patel Jul 02 '18 at 13:59

JavaScript - Adding event listener to an array of objects

4 Answers4