2
\$\begingroup\$

I am a self confessed jQuery novice but I am conscious of learning the correct techniques. When I can see repetition like this I just know that there must be a better way.

jsFiddle

$(document).ready(function() {
    var project1 = $('section').eq(0).offset();
    var project2 = $('section').eq(1).offset();
    var project3 = $('section').eq(2).offset();
    var project4 = $('section').eq(3).offset();
    var project5 = $('section').eq(4).offset();
    var project6 = $('section').eq(5).offset();
    var $window = $(window);

    $window.scroll(function() {
        if ( $window.scrollTop() >= project1.top) {
            $("header").removeClass().addClass("project1");
        }
        if ( $window.scrollTop() >= project2.top ) {
            $("header").removeClass().addClass("project2");
        }
        if ( $window.scrollTop() >= project3.top ) {
            $("header").removeClass().addClass("project3");
        }
        if ( $window.scrollTop() >= project4.top ) {
            $("header").removeClass().addClass("project4");
        }
        if ( $window.scrollTop() >= project5.top ) {
            $("header").removeClass().addClass("project5");
        }
        if ( $window.scrollTop() >= project6.top ) {
            $("header").removeClass().addClass("project6");
        }
    });         
});
\$\endgroup\$
-1
\$\begingroup\$
  • Since section elements are being retrieved in a sequence, a for loop can be used.
  • Since the scroll event can happen frequently, store the header selection in a variable outside of the event.
  • $window.scrollTop() is called multiple times, making it a good candidate to be stored in a variable.

Condensed version of the code:

$(document).ready(function() {
    var $header = $("header");
    var numberOfSections = 6;   
    var sectionOffsets = [];

    for(var i = 0; i < numberOfSections + 1; i++) {
        sectionOffsets.push($('section').eq(i).offset());            
    }            

    $(window).scroll(function() {
        var scrollTop = $(this).scrollTop();            

        for(var i = 0; i < numberOfSections + 1; i++) {
             if(scrollTop > sectionOffsets[i].top) {
                $header.removeClass().addClass("project" + (i + 1));                         
             }
        }
    });
});
| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ Is it necessary to name the number of sections? Could that be removed so that it could be any number? \$\endgroup\$ – 2ne Jan 10 '15 at 10:16
  • \$\begingroup\$ I did it by using section.length jsfiddle.net/o44yy3mg/3 \$\endgroup\$ – 2ne Jan 10 '15 at 10:48
  • \$\begingroup\$ I am still gettin an error that says that it cannot read top of undefined \$\endgroup\$ – 2ne Jan 10 '15 at 10:55
  • \$\begingroup\$ Hello, I edited the code so that the undefined error would no longer occur in the fiddle provided. See the updated version at jsfiddle.net/o44yy3mg/3. To summarize, you don't have to add 1 to the number of iterations the loop will run when selecting by the tag 'section'. \$\endgroup\$ – digita1-anal0g Jan 11 '15 at 21:46
0
\$\begingroup\$

This should do the trick.

$(document).ready(function() {

    var $window = $(window), 
        $sections = $('section'), 
        $header = $('header');

    var elementMatchesViewport = function(data, index, element) {        
        return (data.scrollTop >= $(element).offset().top) ? true : false;
    };

    var setHeaderClassFromElementIndex = function(data, index, element) {
        var classString = 'project' + parseInt($(element).index() + 1, 10);
        data.$header
            .removeClass()
            .addClass(classString);
    };

    var handleScroll = function(data, event) {
        var scrollTop = $(this).scrollTop();
        data.$sections
            .filter($.proxy(elementMatchesViewport, this, {scrollTop: scrollTop}))
            .each($.proxy(setHeaderClassFromElementIndex, this, {$header: $header}));
    };

    $window.on('scroll', $.proxy(handleScroll, this, {$sections: $sections}));

});

Possible further enhancements:

  • Use a zero-based index for your header classes.
  • Debounce the scroll event / use request animation frame, as it's performance intensive.
  • Refactor the condition of elementMatchesViewport() as it matches too many elements.

jsFillde

| improve this answer | |
\$\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.