2
\$\begingroup\$

I'm not proficient at jQuery, however can tinker and get basic functionality working as I would like. The below code is performing a number of functions to change states of some elements in my site, and it is all working correctly.

However, I'm sure it is not the most efficient way of achieving this (fairly simple) functionality, so I'm hoping somebody can help me to make this much leaner and more efficient?

I have commented the code to explain what each line should be achieving, essentially adding and removing classes to a a number of elements. As you can see, a lot of the lines are repeated to accommodate the different states outlined in the comments, but I'm sure something perhaps like .siblings could be used to reduce the amount of code.

Any help greatly appreciated!

JS

$(document).ready(function () {
    $('#menu-button').click(function(){
        // MENU BUTTON STATES
        // If the menu button is active, then remove all classes to return to initial state
        if ($(this).hasClass('active')) {
            $(this).removeClass();
        // When in initial state, add active class
        } else {
            $(this).addClass('active');
        } 

        // OVERLAY STATES
        // If in menu state, clicking menu button removes all #overlay classes to return to initial state
        if ($('#overlay').hasClass('active menu')) {
            $('#overlay').removeClass();
        // If in services state, clicking menu button removes all #overlay classes and add classes to return to menu state
        } else if ($('#overlay').hasClass('active services')) {
            $('#overlay').removeClass().addClass('active menu');
        // If in about state, clicking menu button removes all #overlay classes and add classes to return to menu state
        } else if ($('#overlay').hasClass('active about')) {
            $('#overlay').removeClass().addClass('active menu');
        // If in contact state, clicking menu button removes all #overlay classes and add classes to return to menu state
        } else if ($('#overlay').hasClass('active contact')) {
            $('#overlay').removeClass().addClass('active menu');
        // When in initial state, add classes to change to menu state
        } else {
            $('#overlay').addClass('active menu');
        }
        return false;
    });


    $('.services a').click(function(){
        // When clicking this menu item, remove all #overlay classes and add classes to return to services state
        $('#overlay').removeClass().addClass('active services');
        // Also, return menu button to initial state
        $('#menu-button').removeClass();
        return false;
    });

    $('.about a').click(function(){
        // Same as above but for about state
        $('#overlay').removeClass().addClass('active about');
        $('#menu-button').removeClass();
        return false;
    });

    $('.contact a').click(function(){
        // Same as above but for contact state
        $('#overlay').removeClass().addClass('active contact');
        $('#menu-button').removeClass();
        return false;
    });

    $('.logo').click(function(){
        // When clicking .logo, return everythign to initial state
        $('#overlay').removeClass();
        $('#menu-button').removeClass();
        return false;
    });

});
\$\endgroup\$
1
\$\begingroup\$
  1. Use toggleClass instead of:

    if ($(this).hasClass('active')) {
        $(this).removeClass();
    // When in initial state, add active class
    } else {
        $(this).addClass('active');
    }
    
  2. You can map the event handlers for multiple classes:

    ['about', 'contact', ...].map(cls => {$(`.${cls} a`).click(...)}
    
\$\endgroup\$
3
  • \$\begingroup\$ Hi, toggleClass isn't suitable as multiple elements effect the same items, meaning that toggle doesn't always perform the correct task! \$\endgroup\$ – dungey_140 Jun 9 '17 at 11:53
  • 1
    \$\begingroup\$ Notice that he's not just removing the active class, he's removing all classes. \$\endgroup\$ – Barmar Jun 9 '17 at 22:24
  • \$\begingroup\$ not sure why you care to check the overlay classes, since in every case you are removing all overlay classes and adding the active menu class, unless you want to hide it. toggleclass should suffice here. \$\endgroup\$ – Mike Sep 7 '17 at 18:08

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.