jQuery .on() and adding classes

Question

Why wouldn't this work? I thought if I add a class name to the trigger, upon opening, and then if you click the trigger that now has this class name... we could close it.

// Menu trigger
$('.menu-trigger').on('click', function(e) {
    e.preventDefault();
    $('.menu-container').fadeIn('fast');
    $(this).addClass('open');
});
$('.menu-trigger.open').on('click', function(e) {
    e.preventDefault();
    $('.menu-container').fadeOut('fast');
    $(this).removeClass('open');
});

Answer 1

Event Delegation :

Delegated events have the advantage that they can process events from descendant elements that are added to the document at a later time. By picking an element that is guaranteed to be present at the time the delegated event handler is attached, you can use delegated events to avoid the need to frequently attach and remove event handlers.

 $('parentselector').on('click','childselector',//callbackfunction() 

here, document is just act as a parent for menu class.. It is used to specify .menu-trigger and .menu-trigger.open are individual element inside the document and .on('click' is used for dynamically created element.

$(document).on('click','.menu-trigger' function(e) {
    e.preventDefault();
    $('.menu-container').fadeIn('fast');
    $(this).addClass('open');
});
$(document).on('click','.menu-trigger.open' ,function(e) {
    e.preventDefault();
    $('.menu-container').fadeOut('fast');
    $(this).removeClass('open');
});

Answer 2

Use event delegation for dynamically added element

$(document).on('click',".menu-trigger.open", function(e) {
    e.preventDefault();
    $('.menu-container').fadeOut('fast');
    $(this).removeClass('open');
});

Event delegation allows us to attach a single event listener, to a parent element, that will fire for all descendants matching a selector, whether those descendants exist now or are added in the future.

Answer 3

The reason your code isn't working is that you have two separate handlers and both will fire if the element has the .open class because both selectors in your handlers apply. So, there are a couple of things you could do:

1. Write a single handler for the .menu-trigger and use an if statement to check for the presence of the .open class, such as if($(this).hasClass('open')). If true, do your fadeOut and if false do your fadeIn.
2. Write two handlers like you have but specifically exclude elements with the .open class from the selector in the first one like this: $('.menu-trigger:not(.menu-trigger.open)')...
3. Remove the menu-trigger class and add a class like menu-trigger-close, then you would have two separate handlers: one for $('menu-trigger') and one for $('menu-trigger-close').
4. OR, the best way in my opinion, use the fadeToggle method. It does all of the work for you. It checks if the element is already visible and fades it out and vice-versa. So, you don't have to add any additional classes to keep track.

The easiest way to do this: http://api.jquery.com/fadeToggle/

If all you want to do is toggle between fadeIn and fadeOut, use the fadeToggle method:

$('.menu-trigger').on('click', function(e) {
    e.preventDefault();
    $('.menu-container').fadeToggle('fast');
});