Prototype JS - Moving elements between two containers - what's wrong here?

Question

I'm trying to move elements between two lists. Elements when clicked should move to the other list. Prototype JS code:

document.observe('dom:loaded', function() {

  $$('li.available-item').each(function(element){
    element.observe('click', function(){
      element.removeClassName('available-item');
      element.addClassName('selected-item');
      $('selected-list').insert(element);
    });
  });

  $$('li.selected-item').each(function(element){
    element.observe('click', function(){
      element.removeClassName('selected-item');
      element.addClassName('available-item');
      $('available-list').insert(element);
    });
  });

});

Sample elements:

<ul id="available-list">
  <li class="available-item">Apple</li>
  <li class="available-item">Orange</li>
  <li class="available-item">Grapes</li>
</ul>

<ul id="selected-list">
  <li class="selected-item">Pineapple</li>
  <li class="selected-item">Papaya</li>
</ul>

While it's working the first time I'm clicking on the elements, once an element moves to the other list they don't move to the other (original) list when clicked.

What am I doing wrong here?

Answer 1

The reason is that when you move an item, it's still got the old event handler attached that will just move it again.

This is a classic place for using event delegation. Instead of watching for clicks on the elements, look for clicks on the lists (since clicks bubble up) and then move the relevant element. Something like this:

$('available-list').observe('click', function(event) {
    var li;

    li = event.findElement('li');
    if (li) {
        event.stop();
        li.addClassName('selected-item').removeClassName('available-item');
        $('selected-list').insert(li);
    }
});

...and the converse for selected-list.

You could even use just one handler for both lists:

$('available-list').observe('click', listClick);
$('selected-list').observe('click', listClick);

function listClick(event) {
    var fromType, toType, li;

    // Get the clicked list item
    li = event.findElement('li');
    if (li) {
        // We're handling it
        event.stop();

        // Determine whether moving to the selected or available list
        if (this.id.startsWith("selected")) {
            fromType = "selected";
            totype   = "available";
        }
        else {
            fromType = "available";
            totype   = "selected";
        }

        // Update class names
        li.addClassName(toType + '-item').removeClassName(fromType + '-item');
        $(toType + '-list').insert(li);
    }
});

It gets even simpler if you ditch the classes on the items (see below):

$('available-list').observe('click', listClick);
$('selected-list').observe('click', listClick);

function listClick(event) {
    var targetList, li;

    // Get the clicked list item
    li = event.findElement('li');
    if (li) {
        event.stop();
        targetList = this.id.startsWith("selected") ? "available-list" : "selected-list";
        $(targetList).insert(li);
    }
});

---

Somewhat OT, but you may not need those selected-item and available-item classes at all. With the above, you don't need them to find them anymore, and in your CSS, you can use descendant selectors for styling the elements:

#selected-list li {
    /* ...styles for the `li` elements in the `selected-list` ... */
}
#available-list li {
    /* ...styles for the `li` elements in the `available-list` ... */
}

If you only want to affect lis that are direct children of the lists, use child selectors instead of descendant selectors (note the >):

#selected-list > li {
    /* ...styles for the `li` elements in the `selected-list` ... */
}
#available-list > li {
    /* ...styles for the `li` elements in the `available-list` ... */
}