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

slide out navigation bar seems too simple but it works, is it?

Submitted by: @import:stackexchange-codereview··
0
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?

$(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 $ 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, handler2 are unfortunate names, these function names should convey what the function does ( perhaps slideRight and slideLeft ? )



  • You access $('#menubutton') a number of times, you should cache it with var $menuButton = $('#menubutton') and then access $menuButton, you can use the same technique for the other jQuery calls as wel



  • You are repeating yourself between handler1 and handler2, 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.