Adding DIVs dynamically and attaching events

Question

I'm attaching some DIVs dynamically in JQuery and want to attach each one a click event, this is how I do it

for (var i=0; i<CAItems.length; i++)
    {
        //alert(i);
        $('#UserWorldItems').append(
            $("<div id='ContextItm" + i + "' class='UserItem'><div class='UserItemBox'>" + 
                CAItems[i].Context + "</div><div class='UserItemBox AddPaddingToItem'>" + 
                CAItems[i].Action + "</div></div>").on("click" , function() { alert(i); } )
        );
    }

The DIVs are created but the function that is fired when clicking any of the new divs results in an alert saying "11" (11 is the last value of i). How can I resolve this that each div will show an alert with the right index? (0,1,2,3....11)

possible duplicate of [How do JavaScript closures work?](http://stackoverflow.com/questions/111102/how-do-javascript-closures-work) — Andreas Louv, Oct 30 '13 at 15:29

ccsakuweb · Accepted Answer · 2013-10-30T15:34:42.270

You must to create a function with a param i to create the scope, and return the function you want to execute when the click is done. See that you are executing an anonymous function with param i that is the value of the i from the loop. Like it is a copy the alert will have the correct value. In your case, the for ends with the last value and when you clicked you saw always the same because you was asking for the value of i, now you have a new variable, and a different scope for function.

for (var i=0; i<CAItems.length; i++)
{
    //alert(i);
    $('#UserWorldItems').append(
        $("<div id='ContextItm" + i + "' class='UserItem'><div class='UserItemBox'>" + 
            CAItems[i].Context + "</div><div class='UserItemBox AddPaddingToItem'>" + 
            CAItems[i].Action + "</div></div>").on("click" , function (i) { return function() { alert(i); }; }(i) )
    );
}

ahren · Answer 2 · 2013-10-30T15:38:07.593

0

A better way to approach the problem is to delegate the events to the static parent.

$('#UserWorldItems').on('click', '.UserItem', function(e){
  e.preventDefault();

  // Your code
  // 'this' is set to the .UserItem element that was clicked
});

Of course you won't be able to get the value of i, but you could add that in a data-* attribute (or via the .data() method) if that's what you're really after.

To add the data and store the index:

$("<div id='ContextItm" + i + "' class='UserItem'><div class='UserItemBox'>" + 
        item.Context + "</div><div class='UserItemBox AddPaddingToItem'>" + 
        item.Action + "</div></div>").data('index',i);

And to retrieve:

$(this).data('index');

edited Oct 30 '13 at 15:38

answered Oct 30 '13 at 15:30

ahren

16,803
5
50
70

This is good, but the think the focus is on how to make closures over variables more then how to optimize your code... – Andreas Louv Oct 30 '13 at 15:34
There's no use in leading someone down the garden path if it's the wrong garden. I'm suggesting a better way of doing things. – ahren Oct 30 '13 at 15:36
Why do you think `.data()` is better than variable scope? Because it's slower? – Blue Skies Oct 30 '13 at 15:40
@BlueSkies - Really depends on how many element's the OP is creating. The more event handlers, the slower the whole page would run. The speed for retrieving `.data()` is negligible and the readability is a major bonus. The speed is especially negligible in the above case, where I store the variable using the `.data()` method rather than as a `data-*` attribute, because jQuery stores it internally rather than on the node. – ahren Oct 30 '13 at 15:41
*"The more event handlers, the slower the whole page would run."* How so? – Blue Skies Oct 30 '13 at 15:43
@BlueSkies - http://coding.smashingmagazine.com/2010/04/20/seven-javascript-things-i-wish-i-knew-much-earlier-in-my-career/ http://www.nczonline.net/blog/2009/06/30/event-delegation-in-javascript/ – ahren Oct 30 '13 at 15:45
1

Well, the first article gives no reason. The second article's reason is that more memory usage means a slower page, which can be true. But using `.data()` requires memory as well. Both articles are pretty old. If anything, since we're just talking about an index, I'd probably just put it directly on the element instead of using `.data()`. – Blue Skies Oct 30 '13 at 15:57
`If anything, since we're just talking about an index, I'd probably just put it directly on the element instead of using .data()` This is really notable – Andreas Louv Oct 30 '13 at 16:49
http://jsperf.com/data-method-vs-data-attr - using the `.data()` method is much, *much* faster. – ahren Oct 30 '13 at 17:01

score 0 · Answer 3 · answered Oct 30 '13 at 15:31

0

You need a closure around the variable i:

.on("click" , function(i) { return function() { alert(i); } }(i) )

answered Oct 30 '13 at 15:31

Gigo

3,188
3
29
40

score 0 · Answer 4 · answered Oct 30 '13 at 15:31

I would suggest doing like this:

// Make a function called createDiv,
// The arguments to the function is: item, index
var createDiv = function(item, i) {
    $('#UserWorldItems').append(
        $("<div id='ContextItm" + i + "' class='UserItem'><div class='UserItemBox'>" + 
            item.Context + "</div><div class='UserItemBox AddPaddingToItem'>" + 
            item.Action + "</div></div>"
        ).on("click" , function() {
            alert(i);
        })
    );
};
// Iterate your array and call createDiv:
for (var i=0; i<CAItems.length; i++) {
    createDiv(CAItems[i], i);
}
// Actually now you can use:
// CAItems.forEach(createDiv);

score 0 · Answer 5 · answered Oct 30 '13 at 16:06

There are several ways you can improve the design and efficiency of your code. First, the jQuery append function is roughly 80% slower than concatenating to a string and then appending the entire thing all at once. Then, to clean up your code further, for readability, you can bind the events after appending.

var html = "";

for (var i = 0; i < CAItems.length; i++) {
    html += "<div id='ContextItm" + i + "' class='UserItem'><div class='UserItemBox'>"
        + CAItems[i].Context + "</div><div class='UserItemBox AddPaddingToItem'>"
        + CAItems[i].Action + "</div></div>";
}

$('#UserWorldItems').append(html);
$('#UserWorldItems .UserItem').click(function() {
    var id = this.id;
    var i = id.substring(id.length - 1);
    alert(i);
}

Optionally, you could add a value or item attribute to each div element and then easily access it through jQuery's attr() function, since you probably don't need a completely separate id for each div, but I don't know what you are trying to achieve, so perhaps you do. So, it would look like this:

var html = "";

for (var i = 0; i < CAItems.length; i++) {
    html += "<div id='ContextItm" + i + "' class='UserItem'><div class='UserItemBox'>"
        + CAItems[i].Context + "</div><div class='UserItemBox AddPaddingToItem'>"
        + CAItems[i].Action + " item='" + i + "'</div></div>";
}

$('#UserWorldItems').append(html);
$('#UserWorldItems .UserItem').click(function() {
    alert($(this).attr('item'));
}

Adding DIVs dynamically and attaching events

5 Answers5