4
\$\begingroup\$

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");
    });
\$\endgroup\$
3
\$\begingroup\$

you can try to use a boolean variable to do something like toogle

$(function(){
    var isOpen=false,$page=$("#page");
    $('#menubutton').click(function(e){
        e.preventDefault();
        e.stopPropagation();
        menu();
    });
    function menu(){
        if(isOpen===false){
            open();
        }
        else{
            close();
        }
    }
    function open(){
         $page.animate({left:"200px"},150);
         isOpen=true;
    }
    function close(){
        $page.animate({left:"0"},150);
        isOpen=false;
    }
    $page.click(close);
});        

other changes in the fiddle

  • create a var $page=$("#page")
  • remove $("#menu").css("overflow-y", "hidden"); and set the css directly to auto
  • no need to use .one every time you click the #menubutton
  • change function names to open and close

http://jsfiddle.net/9nT7B/2/

\$\endgroup\$
3
\$\begingroup\$

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
\$\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.