0

I have a small todo list I made for practice. Here is a Fiddle , I cant seem to figure out why the click event that toggle the class that adds and removes the line-through in the todo works on some items, but not others. I checked in the console and for some reason the ones that don't work, when clicked, are firing multiple times. If some one could give me some direction as to why this is happening I would greatly appreciate it.

// Toggle line-through todo
$('.todo-container').on('click', function(){
  $(this).toggleClass('line-through');
  console.log("fired")
})
halfacreyum
  • 297
  • 5
  • 16
  • 5
    It looks like you're running that code inside of another event handler. Do you know what happens when you call `.on` multiple times? – Mike Cluck Sep 16 '16 at 21:50
  • Putting it inside was the only way it seemed to work, because the .todo containers were not recognized outside of the handler. – halfacreyum Sep 16 '16 at 21:55
  • 1
    Sure, because you were producing new elements which needed event handlers attached to them. The problem is you were also attaching event handlers *to ones that already exist.* Check out @DelightedD0D's answer. This is a perfect example of when to use event delegation. – Mike Cluck Sep 16 '16 at 21:56
  • That's because you are adding them dynamically, you need to use event delegation – Wesley Smith Sep 16 '16 at 21:56

1 Answers1

2

Like Mike commented, you're adding a new handler to the elements again on every enter keypress, instead use event delegation

To further explain:

When you add an event handler as shown below, jQuery looks through the DOM and adds the handler directly to any elements that have the todo-container class.

$('.todo-container').on('click', function()...

or

$('.todo-container').click(function()...

An important caveat is that this will only add the handler to elements that currently exist on the page.

You saw this results of this when you realized that the handler did not function on newly created (dynamic) elements.

Not quite understanding why it didnt work, you moved the event bindings into the handler for the keyup effectively calling the binding each time a new element is created. This seemed to work at first but in practice is flawed because this again adds the handler directly to any elements that have the todo-container class. including the elements that already have a handler defined from a previous call.

Fix one, Event delegation (Prefered method)

In the below example we move the bindings back outside the keyup handler and use $('#todos').on('click', '.todo-container', to attach the listener to the '#todos' element (which always exists on the page). Then, any time you click inside that element, it checks if the child you clicked had the class "todo-container" and if so, will fire off your code. This is event delegation. This will catch events on any dynamic element that matches the selector

$(document).on('keypress', function(e) {
  // Add todo
  if (e.which === 13 && $('.input-field').val() != "") {
    $('#todos').prepend(todoTemplate);
    var todo = $('.todo-container');
    $('#todos').children().first().find(todo).html($('.input-field').val());
    $('.input-field').val("");
  }

})



// Remove todo
$('#todos').on('click', '.todo-container-delete', function() {
    $(this).parent().remove();
  })
  // Toggle line-through todo
$('#todos').on('click', '.todo-container', function() {
  $(this).toggleClass('line-through');
  console.log("hello")
})

Fix two, more specific targeting with :last

You could actually leave the bindings inside the keyup handler, if you specifically target only the newly added element like this:

$('#todos .todo-container:last').on('click', function(){

or

$('#todos .todo-container:last').click(function(){

Fix three, (not really recommended but possible) .off()

You could also leave the bindings inside the keyup handler, if you use .off() to remove the handlers from the previous elements before adding it to all the elements again like this:

$('.todo-container').off().on('click', function(){

I'd avoid this method though because if you dont specifically target a handler to remove (see documentation for how), you are removing all handlers applied to that element wich could definitely bite you down the road

Community
  • 1
  • 1
Wesley Smith
  • 19,401
  • 22
  • 85
  • 133
  • Thanks for the details and clear explanation. I was not aware of this, so if I understand what I just read. This isolates the event and prevents the bubbling? Is that right? While I'm much clearer on why this is used, I'm still a little unclear on what was causing the multiple fired events on some items. Is it because the listener was being added multiple times? I apologize if that doesn't make sense. – halfacreyum Sep 16 '16 at 22:33
  • @halfacreyum no prevention of bubbling going on, see my update above for a more complete explanation. – Wesley Smith Sep 16 '16 at 23:30
  • 1
    Wow, really good answer. Thanks for taking the time to thoroughly explain this. Very appreciated! – halfacreyum Sep 16 '16 at 23:41
  • No worries, I would say this concept bites almost all developers starting out with jQuery. – Wesley Smith Sep 16 '16 at 23:47