jQuery Drawer with Tab

Question

I am putting together an animated drawer from scratch for use but also as a learning experience as I am not very strong with jQuery and would really like to get better.

I am hoping that someone here might take a moment to look over my JSFiddle and give me some direction/tips to help me continue to progress?

For example: Is what I put together clean and correct way of doing things? Is creating a function to init the elements for users who have JavaScript as I have done correct? Will what I wrote function well across browsers or are there better ways of achieving this?

jsFiddle

$(document).ready(function() {

    //Hide message if user has javascript
    function initPage() {
        $('#message').css('marginTop', '-60px');
        $('#message').find('.droid').css('bottom', '-60px');
    }

    function slideMessage() {
        var message = $('#message');
        var tab = $('#tab');
        var margin = $('#message').css('marginTop');
        var speed = 300;

        if (margin != "0px") {
            tab.animate({top: "-90px"}, speed);
            message.delay(speed).animate({marginTop: "0px"}, speed);
            message.find('.droid').delay(1000).animate({bottom: "0px"}, 100);
        } else {
            message.find('.droid').delay(1000).css({bottom: "-60px"}, 100);
            message.animate({marginTop: "-60px"}, speed);
            tab.delay(speed).animate({top: "0px"}, speed);
        }
    }

    initPage();

    setTimeout(function() {
        slideMessage();
    }, 1000);

    $('#message .close, #tab').click(function() {
        slideMessage();
    });
});

Answer 1

There are a few issues with your code I've modified it below. Some of the changes I made:

1. cache your jQuery objects

2. reword your slideMessage condition so that it is more readable

3. parameterize the animations and pull them out into methods of their own

4. move your initialization code so that it is all together

Updated JsFiddle

---

$(function () {
    var $message = $('#message'),
        $tab = $('#tab');
    var speed = 300;

    function animateMessage(to) {
        // don't want to animate them right now
        // rather: animate them when this function I'll return is called
        return function () {
            return $message.animate({
                marginTop: to
            }, speed);
        };
    }

    function animateTab(to) {
        return function () {
            return $tab.animate({
                top: to
            }, speed);
        };
    }

    //little helper method to queue animations
    function queue(immediate) {
        var rest = [].splice.call(arguments, 1);
        if (immediate) {
            $.when(immediate()).then(function () {
                queue.apply(window, rest);
            });
        }
    }

    function slideMessage() {
        var tabVisible = $message.css('marginTop') !== '0px';

        if (tabVisible) {
            queue(animateTab('-160px'),
                animateMessage('0px'));
        } else {
            queue(animateMessage('-40px'),
                animateTab('0px'));
        }
    }


    //Hide message if user has javascript
    $message.css('marginTop', '-40px');
    slideMessage();

    $('#message .close, #tab').click(function() {
        slideMessage();
    });
});

---

In response to the comments:

This is very similar to the code you initially put up. I derived it again on a gist using the code you put in your question here (slightly different from the code in the fiddle): https://gist.github.com/3060013/6a4de29001f845c625e2f907752562c7abf9dd85

Clone that repository to see the full history.

1. while your code does work, it has a couple of inefficiencies and the slideMessage has too much going on.

   1. every time you call $('...'), jQuery needs to do some work in the background to figure out what you mean in the string. You can instead cache these items and not need to depend on retrieving them repeatedly being fast. In this case it doesn't make much of a difference, but eventually it will cause your page to be slower.

   2. There is too much extra junk in the way to make it apparent what slideMessage is doing. A good rule of thumb is to wait a week, then if you cannot glance at a statement and know that it is both in the right place in the function and what it is doing then something is too complex. While your code is probably sensible to you right now I am willing to stake the claim that in a few weeks/months when you go to add some other functionality you would waste some time trying to figure out what this is doing.

2. Caching a jQuery object is an expression for introducing a variable that holds the result of the $('...') function and its related forms.

3. Perhaps you shouldn't? I'm not 100% on this yet. I think this part of the code can be further cleaned up. I'm still leaning against combining them. I think something that represents this pseudocode would be best, but I'm not sure how to do that best:

   if tabVisible
       slide tab out then slide message in
   else
       do the opposite of what I did to switch them back
   

That is to say, I don't think the else block should specify the logic of how to do the opposite.

Answer 2

Two small improvements (Not already mentioned):

1. creating an anonymous function that calls another function with no parameters? Not really nessasary.

   setTimeout(function() {
       slideMessage();
   }, 1000);
   

   slideMessage(); has no parameters. Dosen't change and seems a little odd unless sommething else is there in the anonymous function.

   setTimeout(slideMessage, 1000);
   

   should give the same result. (There are occasions when it won't, but in this example there doesn't seem to be any reason.)

2. Such a small thing but i would wrap my code in a closure just for future protection against any changes in my code. On a bigger project this will come in handy. (Or, like me , you might find yourself debugging why something unrelated broke your code.)

   Mostly I see others doing something like:

   ;(function($, window) {
       // ... Your code here
   })(jQuery, window);
   

   The initial ; stops unterminated code from interfering.
   The (function(){ Wraps everything so it doesn't pollute the Global namespace.
   The ($, ...) protects against other frameworks overriding the $ value.