Adding event listeners through for loop

Question

I'm having some issues with some JavaScript code. What I am trying to do is add event listeners to elements in an array.

The problem is that after the code is ran only the last element in the array gets the eventlistener working, the rest of the elements are returning errors.

I'm assuming this is because of the logic I'm using with the for loop and the eventlisteners. What would be an effective way to achieve the desired result?

function addSlider(config) {
var controls = `
        <div id="next"> next </div>
        <div id="previous"> previous </div>`;
var S_wrapper = config.wrapper;
var controls_html = document.createElement("div");
controls_html.setAttribute("id", "ctrl");
controls_html.innerHTML = controls;

var arrayData = config.wrapper.split(",");

for (i = 0; i < arrayData.length; i++) {
    (function(index) {
        document.querySelector(arrayData[i]).appendChild(controls_html);
        var parent = document.querySelector(arrayData[index]);
        console.log(i, index, arrayData[index], parent);

        document.querySelector(arrayData[index]).addEventListener("mouseover", function(){
            this.querySelector("#ctrl").style.display = "block";
        })

        document.querySelector(arrayData[index]).addEventListener("mouseout", function(){
            this.querySelector("#ctrl").style.display = "none";
        })
    })(i);
}

}

Have you considered adding the event listener to the parent node of all the rendered arrayData so you only need one handler instead of the same one for each element? But yes, your issue is a closure related one, I'll try to find the duplicate. — Shilly
– Shilly, Commented Apr 6, 2017 at 9:17

T.J. Crowder · Accepted Answer · 2017-04-06 09:28:05Z

There are a couple of problems:

You're moving the same div to a new parent on each loop iteration. Your controls_html isn't HTML, it's a reference to an HTMLDivElement object. This line:
```
document.querySelector(arrayData[i]).appendChild(controls_html);
```
moves it from whatever parent it has (initially none, but after the first loop it's the element from the previous loop) to the new parent.

If you want a new div in each parent, you need to create a new div each time.
You're using the same ID, ctrl, on more than one element. That's invalid, IDs must be unique. Later, when you use #ctrl, you'll get whatever element the browser chooses to give you (it's unspecified behavior, although in my experience it's always the first one).

So we need to handle both of those issues. Taking a minimal-changes-to-your-approach solution, we move div creation into the loop and add the index to the ID:

function addSlider(config) {
    var controls = `
        <div id="next"> next </div>
        <div id="previous"> previous </div>`;
    var S_wrapper = config.wrapper;

    var arrayData = config.wrapper.split(",");

    for (i = 0; i < arrayData.length; i++) {
        (function(index) {
            var div = document.createElement("div");
            div.setAttribute("id", "ctrl" + index);
            div.innerHTML = controls;
            var parent = document.querySelector(arrayData[index]);
            parent.appendChild(div);

            parent.addEventListener("mouseover", function() {
                this.querySelector("#ctrl" + index).style.display = "block";
            })

            parent.addEventListener("mouseout", function() {
                this.querySelector("#ctrl" + index).style.display = "none";
            })
        })(i);
    }
}

But I'd step back and look at the broader picture. For instance, both mouseover and mouseout bubble, so you could hook those on a parent element and handle this with a single handler. There's also probably no reason for that ID at all; we could identify the child div with a class or similar and seach only within parent for it. You could replace the loop and the function within it with Array#forEach. That sort of thing.

Side note: I notice you're using a template literal (the thing you're assigning to controls), which is a feature introduced in ES2015 (aka "ES6"), but not using any of the other ES2015 features. So just flagging that up in case you didn't realize...

epoch · Accepted Answer · 2017-04-06 09:26:19Z

PS: I'm going to answer the problems in your current code and not talk about design/refactoring (others already commented on this)

Your main problems are using id's instead of classes. remember that only one id can exist within a document, otherwise there is no way to refer to them.

Additionally, you are also reusing the controls_html element, you should create a new one for each element you are adding to. (again, in this situation)

I have created a fiddle with a working copy of your code

function addSlider(config) {
    var controls = `
        <div class="next"> next </div>
        <div class="previous"> previous </div>`;

    var S_wrapper = config.wrapper;
    var arrayData = config.wrapper.split(",");

    for (i = 0; i < arrayData.length; i++) {
        (function(index) {
            var controls_html = document.createElement("div");
            controls_html.setAttribute("class", "ctrl");
            controls_html.style.display = 'none';
            controls_html.innerHTML = controls;

            document.querySelector(arrayData[index]).appendChild(controls_html);
            var parent = document.querySelector(arrayData[index]);

            document.querySelector(arrayData[index]).addEventListener("mouseover", function() {
                this.querySelector(".ctrl").style.display = "block";
            })

            document.querySelector(arrayData[index]).addEventListener("mouseout", function() {
                this.querySelector(".ctrl").style.display = "none";
            })
        })(i);
    }
}

addSlider({
    wrapper: '#a,#b,#c'
})

<div id="a">
    1
</div>
<div id="b">
    2
</div>
<div id="c">
    3
</div>

Collectives™ on Stack Overflow

Adding event listeners through for loop

2 Answers 2

Comments

Comments

Your Answer

Hot Network Questions

Collectives™ on Stack Overflow

2 Answers 2

Comments

Comments

Your Answer

Sign up or log in

Post as a guest

Related