Vanilla javascript for loop inside another for loop adding same class to all divs

Question

I've been look at this piece of javascript for hours and can't determine why every time i hover over rewardImg, it adds the class open to every offer div.

Any ideas? I understand it has something to do with closures or maybe i need to put it into an array?

var offer = document.querySelectorAll(".offer");
var rewardImg = document.querySelectorAll(".rewardImg");

for(var i = 0; i < rewardImg.length; i++) {
    rewardImg[i].addEventListener("mouseover", function() {
        for(var j = 0; j < offer.length; j++) {
            offer[j].classList.add("open");
        }
    });
}

for(var i = 0; i < rewardImg.length; i++) {
    rewardImg[i].addEventListener("mouseout", function() {
        for(var j = 0; j < offer.length; j++) {
            offer[j].classList.remove("open");
        }
    });
}

You iterate over the whole list of `offer` so why do you expect that it should only add it to certain elements of that list? — t.niese, Jul 25 '18 at 11:38
@eisbehr Using `i` in the callback won't work as the `i` is the same for all callbacks, because of the closure. — t.niese, Jul 25 '18 at 11:47
If you already asked the [question](https://stackoverflow.com/questions/51510973) earlier, and it was closed as duplicated and you re-ask it again, then you should mention that you already asked it, and explain why you think that it is a different problem. It clearly is but you should mention the previous question. — t.niese, Jul 25 '18 at 11:48
Possible duplicate of [JavaScript closure inside loops – simple practical example](https://stackoverflow.com/questions/750486/javascript-closure-inside-loops-simple-practical-example) — skyline3000, Jul 25 '18 at 12:21
@skyline3000 the OP does **not** use the `i ` within the callback. In the current form it is not a closure problem. — t.niese, Jul 26 '18 at 04:08

score 0 · Accepted Answer · answered Jul 25 '18 at 12:01

Based on your original question: What you need to do is perform the selection based on the rewardImg.

var rewardImgs = document.querySelectorAll(".rewardImg");

for (var i = 0; i < rewardImgs.length; i++) {
  rewardImgs[i].addEventListener("mouseover", function() {
        var offer = this.parentElement.querySelector(".offer");
        offer.classList.add("open");
   });

   rewardImgs[i].addEventListener("mouseout", function() {
        var offer = this.parentElement.querySelector(".offer");
        offer.classList.remove("open");
   });

}

.rewardImg {
  width: 200px;
  height: 50px;
  background-color: red;
}

.offer {
  width: 200px;
  height: 50px;
  background-color: yellow;
}

.offer.open {
background-color: green;
}

<div>
<div class="offer">
</div>
<div class="rewardImg">
</div>
</div>

<div>
<div class="offer">
</div>
<div class="rewardImg">
</div>
</div>

<div>
<div class="offer">
</div>
<div class="rewardImg">
</div>
</div>

This works because in the callback this is bound to the element that triggers the event (in this case: the element being hovered), hence we can find other DOM elements from there.

thank you so much that is exactly what I was looking for! makes so much sense. thanks for taking the time to explain it as well! — Jessica, Jul 25 '18 at 12:24

score 0 · Answer 2 · answered Jul 25 '18 at 12:21

Note that you are adding the class to all available offer via the for loop so make sure you add that to only the required elements.

Vanilla and no for loop solution will be something like -

document.addEventListener('mouseover', function(e) {
  if (e.target.className == 'offer') {
    var
      seq = '.rewardImg[data-seq="' + e.target.dataset.seq + '"]';
    
    var selected = document.querySelector(seq);
    selected.classList.add("open");
  }
});


document.addEventListener('mouseout', function(e) {
  if (e.target.className == 'offer') {
    var
      seq = '.rewardImg[data-seq="' + e.target.dataset.seq + '"]';
    
    var selected = document.querySelector(seq);
    selected.classList.remove("open");
  }
});

.open {
  background-color:red;
}

<div class='offer' data-seq='1'>1</div>
<div class='offer' data-seq='2'>2</div>
<div class='rewardImg' data-seq='1'>11</div>
<div class='rewardImg' data-seq='2'>22</div>

Make a note that all that javascript must go in on an on-load event

thank you for your help. yes i do put my code into an onload event. just didnt add that into the question =) — Jessica, Jul 25 '18 at 12:26

Vanilla javascript for loop inside another for loop adding same class to all divs

2 Answers2