1
\$\begingroup\$

I wrote this jQuery code as I could for a dropdown menu.

But I'm sure there is a way to make this code shorter.

$(document).ready(function() {
    $('#show-menu-1').on('click touch', openSubMenu);
    $('#close-menu-1').on('click touch', closeSubMenu);

    $(document).on('click touch', function(event) {
        if (!$(event.target).closest('#menu-1').length) {
            $('#menu-1 > .main-menu__submenu').removeClass('main-menu__submenu--active');
            $('#menu-1').removeClass('main-menu__item--active');
        }
    });

    function openSubMenu() {
        $('#menu-1 > .main-menu__submenu').addClass('main-menu__submenu--active');
        $('#menu-1').addClass('main-menu__item--active');
    };

    function closeSubMenu() {
        $('#menu-1 > .main-menu__submenu').removeClass('main-menu__submenu--active');
        $('#menu-1').removeClass('main-menu__item--active');
    };
});

$(document).ready(function() {
    $('#show-menu-2').on('click touch', openSubMenu);
    $('#close-menu-2').on('click touch', closeSubMenu);

    $(document).on('click touch', function(event) {
        if (!$(event.target).closest('#menu-2').length) {
            $('#menu-2 > .main-menu__submenu').removeClass('main-menu__submenu--active');
            $('#menu-2').removeClass('main-menu__item--active');
        }
    });

    function openSubMenu() {
        $('#menu-2 > .main-menu__submenu').addClass('main-menu__submenu--active');
        $('#menu-2').addClass('main-menu__item--active');
    };

    function closeSubMenu() {
        $('#menu-2 > .main-menu__submenu').removeClass('main-menu__submenu--active');
        $('#menu-2').removeClass('main-menu__item--active');
    };
});
\$\endgroup\$
  • \$\begingroup\$ We could probably do a better job of reviewing your code if you also included the corresponding HTML. You can use Ctrl-M in the question editor to include it. \$\endgroup\$ – 200_success Nov 5 '15 at 15:19
1
\$\begingroup\$

You can make this shorter by modifying your html to have the id in a data attribute and using classes, but this is what I have based on the info you've given.

$(function() {

    function initializeMenu(id) {
       $('#show-menu-' + id).on('click touch', function() { toggleMenu(id, true); });
       $('#close-menu-' + id).on('click touch', function() { toggleMenu(id, false); });

       $(document).on('click touch', function(event) {
            if (!$(event.target).closest('#menu-' + id).length) {
                toggleMenu(id, false);
            }
       });
    }

    function toggleMenu(id, open) {
        $('#menu-' + id + ' > .main-menu__submenu').toggleClass('main-menu__submenu--active', open);
        $('#menu-' + id).toggleClass('main-menu__item--active', open);
    }

    for(var i = 1; i <= 2; i++) {
        initializeMenu(i);
    }
});
\$\endgroup\$
  • \$\begingroup\$ This is what I was looking for! Thank you) \$\endgroup\$ – walterdesign Nov 5 '15 at 15:17
0
\$\begingroup\$

Functions

Make "#show-menu-2" and "#close-menu-2" parameters and turn the contents of your $(document).ready() into it's own function so that you may call it again. That'll instantly reduce the code in half. Then you'd just call thatFunction("#show-menu-2", "#close-menu-2") and thatFunction("#show-menu-1", "#close-menu-1").

Redundancy

Your click touch event is calling code identical to your closeSubMenu function. Just call closeSubMenu();. Parameterizing it in a similar fashion to the above example will allow you to do so.

\$\endgroup\$
  • \$\begingroup\$ having function initializeMenu(whichOne) { ... } initializeMenu(1); initializeMenu(2); would make more sense, only the numerical suffix appears to differ between the two ready handlers. I think it would be more useful to show the code changes you are suggesting, rather than describe them. \$\endgroup\$ – AD7six Nov 5 '15 at 15:03
  • \$\begingroup\$ I'm a fan of flexibility when it comes to functions. So I lean towards suggesting the whole selector be the parameter so that you may reuse this function elsewhere. Not necessarily in the same project with the same styles. Might be overkill for this situation though, not sure. \$\endgroup\$ – Shelby115 Nov 5 '15 at 15:05
  • \$\begingroup\$ There's flexiblity and then there's YAGNI ;). \$\endgroup\$ – AD7six Nov 5 '15 at 16:11

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.