views:

1130

answers:

6

I'm generating an unordered list through javascript (using jQuery). Each listitem must receive its own event listener for the 'click'-event. However, I'm having trouble getting the right callback attached to the right item. A (stripped) code sample might clear things up a bit:

for(class_id in classes) {
    callback = function() { this.selectClass(class_id) };
    li_item = jQuery('<li></li>')
       .click(callback);
}

Actually, more is going on in this iteration, but I didn't think it was very relevant to the question. In any case, what's happening is that the callback function seems to be referenced rather than stored (& copied). End result? When a user clicks any of the list items, it will always execute the action for the last class_id in the classes array, as it uses the function stored in callback at that specific point.

I found dirty workarounds (such as parsing the href attribute in an enclosed a element), but I was wondering whether there is a way to achieve my goals in a 'clean' way. If my approach is horrifying, please say so, as long as you tell me why :-) Thanks!

+1  A: 

why can't you generate them all and then call something like

$(".li_class").click(function(){ this.whatever() };

EDIT:

If you need to add more classes, just create a string in your loop with all the class names and use that as your selector.

$(".li_class1, .li_class2, etc").click(function(){ this.whatever() };
contagious
Hmm... I need to 'attach' a different class_id to every listitem. Am I missing out on an easy way to do this using your approach?
JorenB
you attach the *same* class to each `li`. `this` will be the specific item that was clicked.
geowa4
the only problem i have is iterating twice over the same set. once to create, again to add the handler.
geowa4
+1  A: 

My javascript fu is pretty weak but as I understand it closures reference local variables on the stack (and that stack frame is passed around with the function, again, very sketchy). Your example indeed doesn't work because each function keeps a reference to the same variable. Try instead creating a different function that creates the closure i.e.:

function createClosure(class_id) {
  callback = function() { this.selectClass(class_id) };
  return callback;
}

and then:

for(class_id in classes) {
  callback = createClosure(class_id);
  li_item = jQuery('<li></li>').click(callback);
}

It's a bit of a kludge of course, there's probably better ways.

wds
Looks promising... I'll probably have to move somewhere in that direction, although, as you said, it still 'feels' a little weird...
JorenB
+2  A: 

This is a classic "you need a closure" problem. Here's how it usually plays out.

  1. Iterate over some values
  2. Define/assign a function in that iteration that uses iterated variables
  3. You learn that every function uses only values from the last iteration.
  4. WTF?

Again, when you see this pattern, it should immediately make you think "closure"

Extending your example, here's how you'd put in a closure

for ( class_id in classes )
{
  callback = function( cid )
  {
    return function()
    {
      $(this).selectClass( cid );
    }
  }( class_id );
  li_item = jQuery('<li></li>').click(callback);
}

However, in this specific instance of jQuery, you shouldn't need a closure - but I have to ask about the nature of your variable classes - is that an object? Because you iterate over with a for-in loop, which suggest object. And for me it begs the question, why aren't you storing this in an array? Because if you were, your code could just be this.

jQuery('<li></li>').click(function()
{
  $(this).addClass( classes.join( ' ' ) );
});
Peter Bailey
+1  A: 

This is a better cleaner way of doing what you want.

Add the class_id info onto the element using .data().

Then use .live() to add a click handler to all the new elements, this avoids having x * click functions.

for(class_id in classes) {
    li_item = jQuery('<li></li>').data('class_id', class_id).addClass('someClass');
}

//setup click handler on new li's
$('li.someClass').live('click', myFunction )

function myFunction(){
   //get class_id
   var classId = $(this).data('class_id');
   //do something
}
redsquare
Sweet. This actually works in a super awesome way. Thanks for pointing this out to me. Now: it only seems to take jQuery(cssselector). If I use list.children().live(blabla), in which 'list' is a jQuery object of the <ul>, it won't work. Is there a reason for this?
JorenB
not tried it with .children() but I think it needs an actual selector as it uses event delegation on the document then checks to see if the selector is a match for the event. You could revert to use normal event delegation on the ul
redsquare
A: 

Or you can attach the class_id to the .data() of those list items.

$("<li />").data("class_id", class_id).click(function(){
    alert("This item has class_id "+$(this).data("class_id"));
});

Be careful, though: You're creating the callback function anew for every $("<li />") call. I'm not sure about JavaScript implementation details, but this might be memory expensive. Instead, you could do

function listItemCallback(){
     alert("This item has class_id "+$(this).data("class_id"));
}

$("<li />").data("class_id", class_id).click(listItemCallback);
AKX
+3  A: 

Your code:

for(class_id in classes) {
    callback = function() { this.selectClass(class_id) };
    li_item = jQuery('<li></li>')
                        .click(callback);
}

This is mostly ok, just one problem. The variable callback is global; so every time you loop, you are overwriting it. Put the var keyword in front of it to scope it locally and you should be fine.

EDIT for comments: It might not be global as you say, but it's outside the scope of the for-loop. So the variable is the same reference each time round the loop. Putting var in the loop scopes it to the loop, making a new reference each time.

geowa4
Right now, it's 'initialized' just before the for-in-loop, using the `var` keyword. You mean it would work fine if I put this `var`-keyword inside the loop?
JorenB
pretty much. it might not be global as you say, but its outside the scope of the for-loop. so the variable is the same reference each time round the loop. putting var *in* the loop scopes it to the loop, making a new reference each time.
geowa4
Cool. I guess this would be the solution when using my approach. The live()-way seems to work great too, however. Is there such a thing as a 'better' one of the two?
JorenB
less events == better
redsquare