HiveBrain v1.2.0
Get Started
← Back to all entries
patternjavascriptMinor

Handling drop-down menus for different sites

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
handlingmenussitesdropdifferentdownfor

Problem

I have a JavaScript/jQuery function that handles the drop-down menus for several different sites. I am a new to JavaScript and jQuery and would love any input on how to clean up my code. Essentially, I am asking how to make this script more efficient and less clunky, so I can improve performance and write better code.


  

   

  
  

 

  
           
  

   

 $(function() {

    dropdownSlide('.ppt-menu', 'ul.sub-menu', 5000, fade)

})

    

    

      Home
      About 

        

           Approach
        Industries
        Services
        Bio

    

Projects 

    

        UCCS
        The Dixon Collective

    

Projectsttow 

    

        UCCS
        The Dixon Collective

    

Contact




function dropdownMenu(nav, dropdown, speed, type) {

var nav = $(nav), dropdown = $(dropdown);

var parentItem = $(nav).find('li').has(dropdown);

$(parentItem).find(dropdown).hide();

if( type === slide ) {

    $(parentItem).mouseenter(function () {
        $('li').find(dropdown).hide();
        $(this).find(dropdown).stop(true, true).slideDown(speed);
    }).mouseleave(function () {
        $(this).find(dropdown).delay(500).slideUp(speed);
    });

}

else if ( type === fade ) {

    $(parentItem).mouseenter(function () {
        $('li').find(dropdown).hide();
        $(this).find(dropdown).stop(true, true).fadeIn(speed);
    }).mouseleave(function () {
        $(this).find(dropdown).delay(500).fadeOut(speed);
    });

}

else if( type === show) {

    $(parentItem).mouseenter(function () {
        $('li').find(dropdown).hide();
        $(this).find(dropdown).stop(true, true).show(speed);
    }).mouseleave(function () {
        $(this).find(dropdown).delay(500).hide(speed);
    });

}

else {
    $(parentItem).mouseenter(function () {
        $('li').find(dropdown).hide();
        $(this).find(dropdown).stop(true, true).show();
    }).mouseleave(function () {
        $(this).find(dropdown).delay(500).hide();
    });
}

 }

Solution

You've made a few mistakes.

Firstly you used $(parentItem) and $(nav) when you don't need to, because they are already jQuery objects.

Secondly you copy pasted 4 blocks of similar codes without abstracting it into a function.

Then the function itself could have been optimised, by removing the $("li").find(dropdown) call outside the event handlers.

function dropdownMenu(nav, dropdown, speed, type) {

    var nav = $(nav),
        dropdown = $(dropdown);

    var parentItem = nav.find('li').has(dropdown);

    parentItem.find(dropdown).hide();

    function action (enter, leave, speed) {
        var all = $('li').find(dropdown);

        parentItem.mouseenter(function() {
            all.hide();
            $(this).find(dropdown).stop(true, true)[enter](speed);
        }).mouseleave(function() {
            $(this).find(dropdown).delay(500)[leave](speed);
        }); 
    }

    switch (type) {
        case "slide":
            action ("slideDown", "slideUp", speed); break;
        case "fade":
            action ("fadeIn", "fadeOut", speed); break;
        case "show":
            action ("show", "hide", speed); break;
        default: 
            action ("show", "hide"); break;
    }
}


The rest of the optimisations depend on the HTML markup.

Code Snippets

function dropdownMenu(nav, dropdown, speed, type) {

    var nav = $(nav),
        dropdown = $(dropdown);

    var parentItem = nav.find('li').has(dropdown);

    parentItem.find(dropdown).hide();

    function action (enter, leave, speed) {
        var all = $('li').find(dropdown);

        parentItem.mouseenter(function() {
            all.hide();
            $(this).find(dropdown).stop(true, true)[enter](speed);
        }).mouseleave(function() {
            $(this).find(dropdown).delay(500)[leave](speed);
        }); 
    }

    switch (type) {
        case "slide":
            action ("slideDown", "slideUp", speed); break;
        case "fade":
            action ("fadeIn", "fadeOut", speed); break;
        case "show":
            action ("show", "hide", speed); break;
        default: 
            action ("show", "hide"); break;
    }
}

Context

StackExchange Code Review Q#6079, answer score: 3

Revisions (0)

No revisions yet.