2
\$\begingroup\$

I wrote the following to create an animation which collapses a navigation bar, load new content for it and expands it again. Meanwhile, the rest of the page content fades out, loads and fades in again.

It works and looks good, and is even timed well. But my code is one hell of a mess. How could I have done better?

$(function() {
var newHash      = "",
    $NavmainContent = $("#contentNavigationWrapper"),
    $mainContent = $(".contentWrapper"),
    $first       = $('#currentSection'),
    $second      = $('#project-tools'),
    $third       = $('#project-pagetools'),
    $fourth      = $('#cp'),
    $pageWrap    = $("#page-wrap"),
    baseHeight   = 0,
    $el;

$pageWrap.height($pageWrap.height());
baseHeight = $pageWrap.height() - $NavmainContent.height();

$("nav").delegate("a", "click", function() {
    _link = $(this).attr("href");
    history.pushState(null, null, _link);
    loadContent(_link);
    return false;
});

function loadContent(href){

    $mainContent
        .find("#guts")
        .fadeOut(1000, function() {
            $mainContent.hide().load(href + " #guts", function() {
                $mainContent.fadeIn(1000);

            });
        });

    $NavmainContent
        .find('#inner-cn')
        .find("#project-pagetools,#project-tools,#cp,#currentSection")
        .animate({
            'width': '0'
        },800);

    setTimeout(function(){
        $first.load(href + " #currentSection span");
        $second.load(href + " #project-tools li");
        $third.load(href + " #project-pagetools li");
        $fourth.load(href + " #cp span");
    },900);

    setTimeout(function(){

        var l = $("#project-tools li").length ;

        l = 100 / l ;
        l = l + '%';
        $("#project-tools li").css({
            'width': l
        });

        $NavmainContent
            .find("#project-pagetools,#project-tools")
            .animate({
                'width': '15%'
            },800);

        $NavmainContent
            .find("#currentSection")
            .animate({
                'width': '10%'
            },800);

        $NavmainContent
            .find("#cp")
            .animate({
                'width': '55%'
            },800);

    },1100);

}

});
\$\endgroup\$
1
\$\begingroup\$

Here are some quite general pointers on your code. If you put up a fiddle with everything working, I might be able to play around and re-write this for you. But anyways, I've included my comments directly into the code to provide some context:

$(function() {//Is this what wraps your entire code, or just this part of it?
    var newHash      = "",
        $NavmainContent = $("#contentNavigationWrapper"),
        $mainContent = $(".contentWrapper"),
        $first       = $('#currentSection'),
        $second      = $('#project-tools'),
        $third       = $('#project-pagetools'),
        $fourth      = $('#cp'),
        $pageWrap    = $("#page-wrap"),
        baseHeight   = 0,
        $el; //You define "el" but don't use it anywhere.

    $pageWrap.height($pageWrap.height()); //Da fuck? I don't get this lol. You set the height of the element to its own height?
    baseHeight = $pageWrap.height() - $NavmainContent.height(); //I would just include a pageWrapHeight variable since you use often.

    $("nav").delegate("a", "click", function() { //.delegate is deprecated since 1.7, use .on() instead: $("nav").on("click", "a", callback(){});
        _link = $(this).attr("href");
        history.pushState(null, null, _link);
        loadContent(_link);
        return false; //Here you can just e.preventDefault(); instead. Don't forget to pass the "e" for event into the callback.
    });

    function loadContent(href){

        $mainContent
            .find("#guts")
            .fadeOut(1000, function() {
                $mainContent.hide().load(href + " #guts", function() {
                    $mainContent.fadeIn(1000);
                });
            });

        $NavmainContent
            .find('#inner-cn')
            .find("#project-pagetools,#project-tools,#cp,#currentSection")
            .animate({
                'width': '0'
            },800);

            //The method ".load()" calls the method ".ajax()", passing in the stuff you put there.
            //Why not save a few function calls and just use the .ajax() method directly?

            //Instead of using this timeout, you should check out ajax a little more.
            //You'll find that it has a nice little thing called Deffered already built into the ajax method.
            //Also jQuery has a queue which you can manipulate the order of stuff that happens etc., animation in particular.
        setTimeout(function(){
            $first.load(href + " #currentSection span");
            $second.load(href + " #project-tools li");
            $third.load(href + " #project-pagetools li");
            $fourth.load(href + " #cp span");
        },900);

        //Same thing here.
        //You could do this all with a deffered promise.
        //Also worth checking out is Pub/Sub
        //Basically when set up, you can publish, "Hey I'm done doing this stuff" to another part of your JS that is "subscribed" to that,
        //and continue with your other stuff only when that gets published.
        setTimeout(function(){

            var l = $("#project-tools li").length; //I don't personally like using single character variable names.
            //That can be a nightmare to come back to in the future if you have to make changes or include new functionality.
            //But then again that's just me. Don't shoot yourself over that.

            l = 100 / l ;
            l = l + '%';
            $("#project-tools li").css({
                'width': l
            });

            $NavmainContent
                .find("#project-pagetools,#project-tools")
                .animate({
                    'width': '15%'
                },800);

            $NavmainContent
                .find("#currentSection")
                .animate({
                    'width': '10%'
                },800);

            $NavmainContent
                .find("#cp")
                .animate({
                    'width': '55%'
                },800);
        },1100);
    }
});
\$\endgroup\$
  • \$\begingroup\$ Thank you very much! I will provide a fiddle where the problems you mentioned above will be fixed later this evening, when i am finally home :) \$\endgroup\$ – Wottensprels Apr 18 '13 at 14:42

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.