1
\$\begingroup\$

I wrote simple jQuery code and I want suggestions as to how I can do it better and shorter.

Basically the following code is used to change the menu icon on each click event and also hide and display menu accordingly. If I did it correctly, then fine, but is there any way to make the code shorter and faster?

if ( $('.site-topbar').length ) {
    // prepend menu icon
    $('.nav-topbar').prepend('<span class="nav-bars responsive-menu"><i class="ion-android-menu"></i></span>');

    // hide navigation menu
    $('.nav-topbar').find('#menu-topbar-menu').hide();

    if ( $('.responsive-menu').length ) {

        // prepare click event
        $('.responsive-menu').click(function() {

            $(this).find('.ion-android-menu').attr('class', function(){

                // find class='ion-android-menu' exists 
                // then add class='ion-ios-close-empty' if button clicked
                if ( $(this).attr('class') == 'ion-android-menu' ) {
                    $(this).addClass('ion-ios-close-empty');

                    // display menu
                    $('.nav-topbar').find('#menu-topbar-menu').show(800);

                } else {

                    // remove class="ion-ios-close-empty" if condition return false
                    $(this).removeClass('ion-ios-close-empty');

                    // hide menu
                    $('.nav-topbar').find('#menu-topbar-menu').hide(800);
                }
            });
        });
    }
}
\$\endgroup\$
1
\$\begingroup\$

Caching Variables and selectors

Whenever selecting an element on the page, if you are going to select it multiple times, try to set the jQuery element as a variable first, and reuse the same variable.

$('.nav-topbar').prepend('<div></div>');
...
$('.nav-topbar').find('#menu-topbar-menu').hide();

can become:

var $navBar = $('.nav-topbar');
$navBar.prepend('<div></div>');
...
$navBar.find('#menu-topbar-menu').hide();

Caching variables is more performant and can be much easier to read.

You shouldn't have to find by id

IDs are meant to only exist once on the page. By calling .find({selector}), you are effectively saying "Starting from my current element, lets search down the DOM tree until I find something that matches my {selector}"

$('.nav-topbar').find('#menu-topbar-menu').hide();

should be the same as:

$('#menu-topbar-menu').hide();`

If it isn't, then your DOM should be corrected.

== and Type Coercion

As a general rule of thumb, you should always opt for === over == (and conversely opt for !== over !=).

a == b basically means does a sort of equal b. It doesn't take into account the variable's type. "1" == 1 passes as true (even though one is clearly a string, and the other clearly a number).

a === b means does a and b match the same type, and does the value equal each other. In this case, "1" === 1 will return false (because of the difference in type.)

I know this might sound small and nit-picky, but in reality it really isn't. Using == will lead up to unexpected behavior that can be hard to spot.

Additionally, === is faster than ==. === will stop trying to compare two variables if they do not have a matching type.

Simplify Conditional

I want to highlight the following conditional:

if ( $(this).attr('class') == 'ion-android-menu' ) {
    $(this).addClass('ion-ios-close-empty');

    // display menu
    $('.nav-topbar').find('#menu-topbar-menu').show(800);

} else {

    // remove class="ion-ios-close-empty" if condition return false
    $(this).removeClass('ion-ios-close-empty');

    // hide menu
    $('.nav-topbar').find('#menu-topbar-menu').hide(800);
}

You can simplify this conditional to:

var isButtonClicked = $(this).hasClass('ion-android-menu');
$(this).toggleClass('ion-ios-close-empty', isButtonClicked);
$('#menu-topbar-menu').toggle(800);

(Note I strongly recommend using hasClass(...) over .attr('class') == '...', you'll run into less problems in the future this way.

Event Binding

You should try to get into the habit of using $('.parent-class').on('click', '{selector}', function(){}) instead of `$({selector}).click(function(){}).

.click() applies an event handler for every instance of {selector}, whereas the .on() will apply a delegate event handler to .parent-class, and listen for any {selector} within .parent-class. The difference is making 1 event handler compared to making many.

Right here, there shouldn't be too much of a difference. We are assuming that there is only 1 .responsive-menu. But, as you can imagine, using .on() can give huge performance benefits over .click directly.

But, most importantly, by using $({selector}).click(), you are binding the click event to {selector}. Should you remove {selector} and re-add a new {selector} in its place, the click event will be lost. This is not the case with applying the .on() to a parent element.


Overall approach

Hiding and showing menus has been done a lot, and to be honest, you are applying a lot of javascript for something that can be done mostly with css. I'm not aware of your DOM, however you can move the toggle menu behavior (and even the animation) to your CSS and just have a simple click event handler that fires a toggleClass event.

Example:

    var $wrapper = $('.nav-wrappers');
    $wrapper.on('click', '.responsive-menu', function(){
        $wrapper.toggleClass('showTopBar');
    });
.nav-wrappers .menu {
  background: #ccc;
  padding: 0.5em;
  text-align:center;
  transition: all .5s ease;
  overflow:hidden;
}

.responsive-menu {
  cursor:pointer;
}
.responsive-menu:hover {
  background: #ffa
}

/** Animation Stuff **/

.menu-topbar-menu {
  opacity: 0;
  padding: 0;
  height: 0;
}
.nav-wrappers.showTopBar .menu-topbar-menu {
  opacity: 1;
  height: 1em;
}
<script src="https://ajax.googleapis.com/ajax/libs/jquery/1.9.1/jquery.min.js"></script>

<div class="nav-wrappers showTopBar">
    <div class="menu-topbar-menu menu">Top Menu</div>
    <div class="responsive-menu menu">Responsive Menu</div>
</div>

Hope that helps!

\$\endgroup\$
0
\$\begingroup\$

Just because you can use jQuery for something and the code is simple, doesn't mean that you should (or have) to use it. Some browsers like Internet Explorer might actually block JavaScript files on shared hosting (like Google) by default, since they're caught by tracking protection functionality or addons. This might break your page unless the user whitelists it.

What you're trying to achieve can be done by a tiny JavaScript function:

function menu_toggle(node) {
    node._isShown = !node._isShown;
    if (node._isShown)
        node.className = '';
    else
        node.className = 'menu_hidden';
}

Your actual menu just has to include the proper event call as well as some CSS magic:

<div id="menu" class="menu_hidden">
    <div id="menu_button" onclick="menu_toggle(this.parentNode)">Menu</div>
    <div id="menu_content">Some<br />Menu<br />Content</div>
</div>

You can find the full example in this jsFiddle.

\$\endgroup\$

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.