57

I am trying to add an event listener but no result came. I know JavaScript has a hoisting feature but I believe I tried all except the correct solution.

const cbox = document.querySelectorAll(".box");
function doit() {
  for (let i = 0; i < cbox.length; i++){
    cbox[i].classList.add("red");
  }
}
cbox.addEventListener("click", doit, false);

Can somebody spot the mistake I make?

leonheess
  • 16,068
  • 14
  • 77
  • 112
Evan
  • 583
  • 1
  • 5
  • 11
  • Why you're using a loop in `doit()` to add the `red` class? – Andreas Jun 01 '18 at 12:23
  • Because querySelectorAll returns an object soi have to iterate right? – Evan Jun 01 '18 at 12:26
  • 2
    Yes, it returns an array-like object. Next question would be: why you're not using a loop for assigning the event handler? ;) – Andreas Jun 01 '18 at 12:27
  • You mean run the for loop inside the handler or vice versa? I tried them both. – Evan Jun 01 '18 at 12:32
  • 4
    `.querySelectorAll()` returns a `NodeList` which has no `.addEventListener()` method. You have to add the event on every single item in the list, the same way as you're doing it with the class. – Andreas Jun 01 '18 at 12:34

4 Answers4

80

There are some dissimilarities between the code and the link you have provided. There is no function doit() in there.

You have attached addEvenListener to the NodeList in cbox.addEventListener("click",....., you have to loop through the list and attach the event to the current element:

Try the following: 

const cbox = document.querySelectorAll(".box");

 for (let i = 0; i < cbox.length; i++) {
     cbox[i].addEventListener("click", function() {
       cbox[i].classList.toggle("red");
     });
 }
*,
html,
body {
    padding: 0;
    margin: 0;
}

.box {
    width: 10rem;
    height: 10rem;
    background-color: yellowgreen;
    float: left;
    position: relative;
    margin: 0.5rem;
    transition: .5s all;
}

h3 {
    display: block;
    position: absolute;
    top: 50%;
    left: 50%;
    transform: translate(-50%, -50%);
}

.box:not(:first-child) {
    margin-left: 1rem;
}

.red {
    background-color: orangered;
}
<div id="box1" class="box box1">
    <h3>Box 1</h3>
</div>
<div id="box2" class="box box2">
    <h3>Box 2</h3>
</div>
<div id="box3" class="box box3">
    <h3>Box 3</h3>
</div>
<div id="box4" class="box box4">
    <h3>Box 4</h3>
</div>

You can also use Array.prototype.forEach() with arrow function syntax that will allow you to achieve the same with less code:

let cbox = document.querySelectorAll(".box");
cbox.forEach(box => {
  box.addEventListener('click', () => box.classList.toggle("red"));
});
*,
html,
body {
    padding: 0;
    margin: 0;
}

.box {
    width: 10rem;
    height: 10rem;
    background-color: yellowgreen;
    float: left;
    position: relative;
    margin: 0.5rem;
    transition: .5s all;
}

h3 {
    display: block;
    position: absolute;
    top: 50%;
    left: 50%;
    transform: translate(-50%, -50%);
}

.box:not(:first-child) {
    margin-left: 1rem;
}

.red {
    background-color: orangered;
}
<div id="box1" class="box box1">
    <h3>Box 1</h3>
</div>
<div id="box2" class="box box2">
    <h3>Box 2</h3>
</div>
<div id="box3" class="box box3">
    <h3>Box 3</h3>
</div>
<div id="box4" class="box box4">
    <h3>Box 4</h3>
</div>
Mamun
  • 66,969
  • 9
  • 47
  • 59
  • 2
    While this code may answer the question, providing additional context regarding _how_ and/or _why_ it solves the problem would improve the answer's long-term value. – Andreas Jun 01 '18 at 12:27
  • 2
    `.querySelectorAll()` doesn't return a `HTMLCollection` but a `NodeList` – Andreas Jun 01 '18 at 12:33
  • So if its some kind of list it means that we can use a foreach loop also on the element cbox. – Evan Jun 01 '18 at 12:37
  • 1
    @EvangelosK., yes, you are right. You have to attach event listener to each element by using a loop.....thanks. – Mamun Jun 01 '18 at 13:34
  • Thanks. But as far as performance concerns would that event fired 20 times if we have 20 items in the array? One for each element.? – Evan Jun 01 '18 at 13:37
  • @EvangelosK., there is no other way to attach event to all the elements in a list. As far as I know there will not be any performance issues for 20 elements which you can be worry about.....thanks. – Mamun Jun 01 '18 at 13:42
  • How nice of you copying my answer and adding it to yours. Might be worth a read: https://meta.stackoverflow.com/questions/362656/including-other-peoples-answer-into-your-own-accepted-answer – leonheess Mar 15 '22 at 11:09
44

ES6 makes this a bit simpler:

document.querySelectorAll(".box").forEach(box => 
  box.addEventListener("click", () => box.classList.toggle("red"))
)

Example implementation:

document.querySelectorAll(".box").forEach(box =>
  box.addEventListener("click", () => box.classList.toggle("red"))
)
.box {
  width: 5rem;
  height: 5rem;
  background-color: yellowgreen;
  display: inline-block;
}

.box.red {
  background-color: firebrick;
}
<div class="box"></div>
<div class="box"></div>
<div class="box"></div>
leonheess
  • 16,068
  • 14
  • 77
  • 112
14

You can use forEach on the class or use Event delegation.

const cboxes = document.querySelectorAll(".box");
function doit() {
    ... do something ...
}
cboxes.forEach(
  function(cbox) {
   cbox.addEventListener("click", doit,false);
  }
);

Notice that I changed your variable name.

EventDelgation

HTML:

<div id="parent">
  <div id="box1" class="box box1">
    <h3>Box 1</h3>
  </div>
  <div id="box2" class="box box2">
      <h3>Box 2</h3>
  </div>
  <div id="box3" class="box box3">
      <h3>Box 3</h3>
  </div>
  <div id="box4" class="box box4">
      <h3>Box 4</h3>
  </div>
</div>

The JS part:

const parent = document.querySelector("#parent");

parent.addEventListener('click', (e) => {
  e.target.classList.add('red');
  console.log(e.target);
});

Event delegation is much better and it uses fewer resources, as you only use 1 Event listener and no for loop.

Riad ZT
  • 329
  • 1
  • 13
  • 1
    `cboxes.forEach()` won't work with IE: https://developer.mozilla.org/en-US/docs/Web/API/NodeList/forEach#Browser_Compatibility – Andreas Jun 01 '18 at 12:31
  • 2
    `Array.prototype.forEach !== NodeList.prototype.forEach` – Andreas Jun 01 '18 at 13:00
  • easy polyfill fix to make forEach work in <= IE11 is: `NodeList.prototype.forEach = Array.prototype.forEach;` – Felix Geenen Sep 25 '19 at 12:42
  • or you can use the array spread operator like this: `[...document.querySelectorAll(".box")]` – Jason Sperske Jun 30 '21 at 15:39
  • `parent.addEventListener('click', (e) => { const tgt = e.target; if (tgt.classList.contains('box')) tgt.classList.add('red'); console.log(tgt.id); });` – mplungjan Aug 12 '21 at 20:47
4

Rather than iterate through multiple elements and attach a lot of separate listeners, I would recommend using event delegation and creating a single delegated listener at the root level, and then looking for matching elements that raised the event.

document.body.addEventListener("click", function(e) {
  if (e.target.classList.contains("box")) {
    doit();
  }
})

Further Reading

KyleMit
  • 30,350
  • 66
  • 462
  • 664