3
\$\begingroup\$

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

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.

\$\endgroup\$
  • \$\begingroup\$ Hey there Bill, thank you for taking the time to write that out for me. In looking at your code I feel completely lost as I thought my code would be much closer to the "right way" of doing it than it turns out to be. I have a few questions and I hope that you or someone else could help me with them? \$\endgroup\$ – Stefan Jul 6 '12 at 6:23
  • \$\begingroup\$ 1) All together, since what I wrote did work, could you tell me why I should have written it as you did? I am assuming its a performance thing, but I am not sure why or how. \$\endgroup\$ – Stefan Jul 6 '12 at 6:38
  • \$\begingroup\$ 2) Could you tell me where in the code I am caching the jquery object, I am not sure what that is. \$\endgroup\$ – Stefan Jul 6 '12 at 6:38
  • \$\begingroup\$ 3) Since the animations of the sliding drawer and sliding tab are linked to one another and always will be, why should I separate them out into two functions as seen here? \$\endgroup\$ – Stefan Jul 6 '12 at 6:39
3
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ Ok great this is a great answer thanks. I have heard about closures and read a bit on them but understand next to nothing about them. 1) I thought that when calling a function(is method the right term here?) we always needed to have the () at the end, as in slideMessage(). I did not even realize that i could simply write the name of the function. \$\endgroup\$ – Stefan Jul 6 '12 at 6:58
  • \$\begingroup\$ @Stefan think of the function names as variables containing a function. (thats not 100% correct... but who cares for a moment) Now think that the () mean run function inside variable. Now consider: function awesome(){ do awesome stuff}; var pretendAwesome = awesome; pretendAwesome(); Tada! \$\endgroup\$ – James Khoury Jul 6 '12 at 7:10
  • \$\begingroup\$ ok and since the function setTimeout is receiving another function as a parameter, I can simply write the function name as a variable meaning that technically I dont need the (). Is that about right? \$\endgroup\$ – Stefan Jul 6 '12 at 7:24
  • \$\begingroup\$ @Stefan Basically yes. \$\endgroup\$ – James Khoury Jul 6 '12 at 12:24

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.