Dynamic event attachment not working correctly

Question

var smartActionsId = ['smartActions1','smartActions5','smartActions10'];

for (var i in smartActionsId) {
     console.log("smartActionsId ="+smartActionsId[i]);
    $('#' + smartActionsId[i] + ' select').change(function () {
        var value = $(this).val();
        var disableValue;
        var ruleIndex = smartActionsId[i].substr(11);
        console.log("smartActionsId ="+smartActionsId[i]+" i ="+i);
        if (value === '0') {
            disableValue = true;
            onRuleToggle(disableValue, ruleIndex)
        }
        else if (value === '1') {
            disableValue = false;
            onRuleToggle(disableValue, ruleIndex)
        }
    });
}

I'm creating change event dynamically for a multiple switch slider items using the above JavaScript code. But problem I'm facing is, when I click on any switch 'i' value gets replaced with the last value i.e. in smartActionsId I have 3 elements, which ever switch I change it effects for last switch (smartActions10).

Could you please help me resolving this issue?

There's no need for a loop, since all of the callbacks are the same except for the `this` they reference (and your `ruleIndex` var). Try `var ruleIndex = $(this).attr('id').substr(11);` — yoavmatchulsky, Feb 18 '14 at 07:39
@yoavmatchulsky `this` doesn't reference `#smartActionX` but the `select` underneath it. — Ja͢ck, Feb 18 '14 at 07:49

score 2 · Answer 1 · answered Feb 18 '14 at 07:54

Other answers here fixed your problem, but I think you can refactor your code a little and make it much more understandable.

First, I don't like IDs. in your scenario, you have multiple ids which needs to be treated the same. Why not use one mighty class?

Also, ruleIndex calculated from element's ID? smells rotten. If it tells you something about the element, it should be in an attribute or a data-* attribute.

The first bit of code fixes the markup by adding ruleIndex as data attribute and adding a .smart-actionable class. (Maybe you can even move this part to the server-side, to provide yourself with easier markup for JS).

Now, this makes the event handling quite simple.

var smartActionsId = ['smartActions1','smartActions5','smartActions10'];

for (var i in smartActionsId) {
    $('#' + smartActionsId[i])
      .data('ruleIndex', smartActionsId[i].substr(11))
      .addClass('smart-actionable');
}

$('.smart-actionable').on('change', 'select', function() {
    var value = $(this).val();
    var disableValue = (value === '0');
    onRuleToggle(disableValue, $(this).data('ruleIndex'));
});

Hope it will help.

score 1 · Answer 2 · answered Feb 18 '14 at 07:36

You cant use for...in in this case. Please try the code below:

var smartActionsId = ['smartActions1', 'smartActions5', 'smartActions10'];

for (var i = 0; i < smartActionsId.length; i++) {
    console.log("smartActionsId =" + smartActionsId[i]);
    $('#' + smartActionsId[i] + ' select').change(function() {
        var value = $(this).val();
        var disableValue;
        var ruleIndex = smartActionsId[i].substr(11);
        console.log("smartActionsId =" + smartActionsId[i] + " i =" + i);
        if (value === '0') {
            disableValue = true;
            onRuleToggle(disableValue, ruleIndex)
        } else if (value === '1') {
            disableValue = false;
            onRuleToggle(disableValue, ruleIndex)
        }
    });
}

This still exhibits the problem of the value of `i` not being closed over. — Ja͢ck, Feb 18 '14 at 07:50

aleclarson · Accepted Answer · 2014-02-18T07:55:22.277

You don't want to attach event listeners inside a for loop because the variable that tracks the index is used by each loop cycle. If you do that, the i variable will always equal the length of the array minus 1. Use Array.prototype.forEach() instead to prevent that.

var smartActionsId = ['smartActions1','smartActions5','smartActions10'];

smartActionsId.forEach(function (identifier, index) {
  console.log("smartActionsId ="+identifier);
  $('#' + smartActionsId[index] + ' select').change(function () {
    var value = $(this).val();
    var disableValue;
    var ruleIndex = smartActionsId[index].substr(11);
    console.log("smartActionsId ="+smartActionsId[index]+" index ="+index);
    if (value === '0') {
      disableValue = true;
      onRuleToggle(disableValue, ruleIndex)
    }
    else if (value === '1') {
      disableValue = false;
      onRuleToggle(disableValue, ruleIndex)
    }
  });
});

Please Note: IE8 and down does NOT support Array.prototype.forEach().

`Array.forEach()` is not supported in IE8 and below. – Ja͢ck Feb 18 '14 at 07:52 — Ja͢ck, Feb 18 '14 at 07:52
Thanks primostyle.. Its working fine now – Dilip Feb 18 '14 at 07:54 — Dilip, Feb 18 '14 at 07:54

ostapische · Answer 4 · 2014-02-18T08:08:13.697

1

I've always use names like smartActions_1. If you can use it, then in your .change function you can use

// if 'smartActionsId' is global variable
// and if you need to get position in 'smartActionsId' array
var numInArray = $.inArray( this.parentNode.id, smartActionsId );
// this - your select DOM object
var ruleIndex = parseInt( this.parentNode.id.split( "_" )[ 1 ] );

And remember that this in .change function its select which have no id and you must use this.parentNode or $( this ).parent() to get it's holder (I think its div or somethink like that).

@Jack in comments is right: select may not be a direct child. Then you can use this code:

var parent = $( this ).closest( "[id^=smartActions]" );
var numInArray = $.inArray( parent.attr( "id" ), smartActionsId );
var ruleIndex = parseInt( parent.attr( "id" ).split( "_" )[ 1 ] );

edited Feb 18 '14 at 08:08

answered Feb 18 '14 at 07:42

ostapische

1,572
2
12
26

Due to the selector, it may not by `parentNode` you need to look at. – Ja͢ck Feb 18 '14 at 07:47
@Jack I think you not right, can see there http://jsfiddle.net/ostapische/uVSkJ/ – ostapische Feb 18 '14 at 07:56
The selector is `#zyz select`, so a `select` below an `#xyz` but not necessarily a direct descendent (child). – Ja͢ck Feb 18 '14 at 07:59
@Jack, now I understand about what you say. – ostapische Feb 18 '14 at 08:09

Dynamic event attachment not working correctly

4 Answers4