Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

You can see the JS at the end of this post in action here at http://andrew-oh.com/portfolio/timeline/ (It's responsive!)

I've recently picked up javascript/jquery and would love some feedback on how I'm implementing this functionality. As you can see, the li items in the navbar change based on where the user's scroll position is.

Is this an efficient way of implementing this? Am I using best practice?

I've already learned from posting a similar question on reddit to cache objects in variables, that jquery reads selectors from right to left, and that I should use preventDefault() instead return false. Is there anything else I should do to improve my JS/Jquery skills?

Thank you very much!

    jQuery(function ($) {
    "use strict"; //don't know what this means but JSHint tells me to put this here...
    var $decTops = [], $decHeights = [], mobileView = 780, $i = 0, $nav = $("#navigation"), $decadeDivs = $("div.decade"), $timeline = $("#timeline"); // caching various elements in var names

    function coordsUpdate() { // updates where each decade starts and how long the decade's div is everytime the user scrolls/resizes the window and pushes those values to two arrays
        $decTops.length = 0, $decHeights.length = 0;
        $decadeDivs.each(function() {
            $decTops.push($(this).offset().top-51);
            $decHeights.push($(this).outerHeight(true));
        });
        $decTops.push($("#footer-wrapper").offset().top-21); // bugfix: push the footer's position to the array as well to avoid the disappearing navbar bug.
    }

    function decadeCounter() { // counts which decade the user is on based on scrollTop position
        var $currentLoc = $(window).scrollTop();
        while ($currentLoc > $decTops[$i] + $decHeights[$i]) {
            $i++;
        }
        while ($currentLoc > $decTops[0] && $currentLoc < $decTops[$i]) {
            $i--;
        }
        navAdjust($i, $currentLoc);
        //console.log('currentHeight: ' + $(window).scrollTop() + ', decTops: ' + $decTops + ', decHeights: ' + $decHeights);
        //console.log('$i: ' + $i + ', $old: ' + $old);
    }

    function navAdjust($i, $currentLoc) { //fix the navbar to the top and highlight the correct decade in the navbar or fix the decade header to top if using mobile viewport.
        if ($(window).width() >= mobileView) { 
            $timeline.find("div.top-fix").removeClass("top-fix"); //don't want the decade headers on top if using desktop viewport
            if ($currentLoc >= $decTops[0] && $i <= 6) {
                $nav.addClass("fixed-nav").removeClass("nav-width-fix");
                $nav.find("a.active").removeClass("active");
                $nav.find("a:eq(" + $i + ")").addClass("active");
            }
            else { //unfix navbar if user is on top of page or at bottom
                $nav.removeClass("fixed-nav").addClass("nav-width-fix");
                $nav.find("a.active").removeClass("active");
            }
        }
        else {
            if ($currentLoc >= $decTops[0]) { //don't want the navbar on top if using mobile viewport
                $timeline.find(".top-fix").removeClass("top-fix").removeClass("top-fix");
                $timeline.find(".decade:eq(" + $i + ") .dec-banner").addClass("top-fix");
            }
            else {
                $timeline.find("div.top-fix").removeClass("top-fix");
            }
        }
    }
    $nav.on("click", "a", function() { // bring user to specific decade if clicked on in the main navbar
        $nav.find("a.active").removeClass("active");
        var $navlink = $(this).attr("href");
        $('html,body').stop().animate({scrollTop: $($navlink).offset().top},"fast");
        return false;
    });

    $timeline.find(".dec-banner").on("click", "a", function(event) { // bring user to previous or next decade if clicked on in the decade header
        var $navlink = $(this).attr("href");
        if ($(this).parent().hasClass("decade-jump-prev")) {
            $("html,body").stop().animate({scrollTop: $($navlink).offset().top-41},"fast");
        }
        if ($(this).parent().hasClass("decade-jump-next")) {
            $("html,body").stop().animate({scrollTop: $($navlink).offset().top+10},"fast");
        }
        event.preventDefault();
    });

    // add listener
    $(window).on('load orientationchange resize', coordsUpdate);
    $(window).on('scroll load orientationchange resize', decadeCounter);
});
share|improve this question
add comment

1 Answer 1

up vote 5 down vote accepted

Is there anything else I should do to improve my JS/Jquery skills?

Self-invoking Anonymous Function

(function ( $, window, undefined ) {
    doStuff();
})( jQuery, window );

The extra () at the end of the function, determines that it runs itself immediately.

First, we are creating a self-invoking anonymous function to shield ourselves from using global variables. We pass in $, window, and undefined.

The arguments the self invoking function is called with are jQuery and window; nothing is passed in for undefined, so that if we decide to use the undefined keyword within the plugin, “undefined” actually will be undefined. This shields from other scripts potentially assigning a malicious value to undefined, such as true!

Now since ECMAScript 5 the "Immutable undefined" came along which is meant to protect you from exactly that. But as you can see from the answers to this recent question I asked myself, this is not the case yet.

$ is passed as jQuery; we do it this way to ensure that, outside of the anonymous function, $ can still refer to something else entirely, such as Prototype.

Passing the variable for the globally accessible window object allows for more compressed code through the minification processes (which you should be doing, as well).

(function(b,a,c){doStuff()})(jQuery,window); //Same function after being compressed.

Thus every time you refer to window in your code, when compressed it can be replaced with a simple a(or any other letter for that matter) and depending on how much you use it, will improve your performance.

Use-Strict Mode

You should try to understand the reason why a declaration is used and not simply because someone is telling you to. JSHint isn't really a bible you should follow to the death. I'm not saying you shouldn't use it (you probably should use it), I'm saying that it's a good idea to study the "hints" it's giving you, and determine whether or not to use them.

In this case I highly recomend that you do use the strict mode. It will many times force you to write better code if you understand how it works.

As Nicholas C. Zakas puts it:

"Strict mode makes a lot of changes to how JavaScript runs. The intent is to allow developers to opt-in to a “better” version of JavaScript."

Here is the link to his article which might help you understand. http://www.nczonline.net/blog/2012/03/13/its-time-to-start-using-javascript-strict-mode/

share|improve this answer
    
Thank you for the detailed answers! I'll be sure to read up on the resources you linked –  Mister Oh Feb 12 '13 at 20:59
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.