patternjavascriptMinor
Handling drop-down menus for different sites
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
Contactfunction 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
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
The rest of the optimisations depend on the HTML markup.
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.