patternjavascriptMinor
slide out navigation bar seems too simple but it works, is it?
Viewed 0 times
slidebarsimplebutnavigationseemstooworksout
Problem
http://crystalcleanhomes.com/slide/
There are countless scripts that perform this simple slide-out navigation menu but they are all so bloated and the styles are so convoluted that I decided to make my own and it seems almost too simple and too basic so I've come here for criticism. From my testing it even works in IE7 and an older Android tablet (and of course flawlessly in Chrome, iOS, FF, etc.). So please critique this!
Here is a fiddle to more easily see the code: http://jsfiddle.net/9nT7B/
Question: does this meet compliance?
There are countless scripts that perform this simple slide-out navigation menu but they are all so bloated and the styles are so convoluted that I decided to make my own and it seems almost too simple and too basic so I've come here for criticism. From my testing it even works in IE7 and an older Android tablet (and of course flawlessly in Chrome, iOS, FF, etc.). So please critique this!
Here is a fiddle to more easily see the code: http://jsfiddle.net/9nT7B/
Question: does this meet compliance?
$(function(){
$('#menubutton').click(function(e){
e.preventDefault();
e.stopPropagation();
});
function handler1(){
$("#page").animate({left:"200px"},150);
$("#menu").css("overflow-y","auto");
$("#menubutton").one("click",handler2);
}
function handler2(){
$("#page").animate({left:"0"},150);
$("#menu").css("overflow-y","hidden");
$("#menubutton").one("click",handler1);
}
$("#menubutton").one("click",handler1);
$("#page").click(handler2);
$("#menu").css("visibility","visible");
});Solution
From a once over:
-
You should put the closing brace at the level of
-
You should put the closing brace at the level of
$ for readability:$(function () {
$('#menubutton').click(function (e) {
e.preventDefault();
e.stopPropagation();
});
function handler1() {
$("#page").animate({
left: "200px"
}, 150);
$("#menu").css("overflow-y", "auto");
$("#menubutton").one("click", handler2);
}
function handler2() {
$("#page").animate({
left: "0"
}, 150);
$("#menu").css("overflow-y", "hidden");
$("#menubutton").one("click", handler1);
}
$("#menubutton").one("click", handler1);
$("#page").click(handler2);
$("#menu").css("visibility", "visible");
});handler1,handler2are unfortunate names, these function names should convey what the function does ( perhapsslideRightandslideLeft? )
- You access
$('#menubutton')a number of times, you should cache it withvar $menuButton = $('#menubutton')and then access $menuButton, you can use the same technique for the other jQuery calls as wel
- You are repeating yourself between
handler1andhandler2, but it seems that trying to find a common routine would make this code more complex, so I would leave that alone
Code Snippets
$(function () {
$('#menubutton').click(function (e) {
e.preventDefault();
e.stopPropagation();
});
function handler1() {
$("#page").animate({
left: "200px"
}, 150);
$("#menu").css("overflow-y", "auto");
$("#menubutton").one("click", handler2);
}
function handler2() {
$("#page").animate({
left: "0"
}, 150);
$("#menu").css("overflow-y", "hidden");
$("#menubutton").one("click", handler1);
}
$("#menubutton").one("click", handler1);
$("#page").click(handler2);
$("#menu").css("visibility", "visible");
});Context
StackExchange Code Review Q#45995, answer score: 3
Revisions (0)
No revisions yet.