HiveBrain v1.2.0
Get Started
← Back to all entries
patternjavascriptMinor

Multiple uses of $window.scroll

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
windowusesscrollmultiple

Problem

I'm using $window.scroll twice in a jQuery script for different uses. Should I try and combine them?

jQuery(document).ready(function($) {

// Add class .fixed when .site-header hits top of viewport

var distance = $('.site-header').offset().top,
    $window = $(window);

$window.scroll(function() {
    if ( $window.scrollTop() >= distance ) {
        $(".site-header").addClass("fixed");
    }
    if ( $window.scrollTop() = 200) {
        $("body").addClass("scrolled");
    }
    if (scroll <= 200) {
        $("body").removeClass("scrolled");
    }
});

});

Solution

You have an extra if statement in these that should be structured as an if/else statement, or even an if/else if statement

var distance = $('.site-header').offset().top,
    $window = $(window);

$window.scroll(function() {
    if ( $window.scrollTop() >= distance ) {
      $(".site-header").addClass("fixed");
    }
    if ( $window.scrollTop() < distance ) {
      $(".site-header").removeClass("fixed");
    }
});


You should write it like this instead:

$window.scroll(function() {
    if ( $window.scrollTop() >= distance ) {
        $(".site-header").addClass("fixed");
    } else if ( $window.scrollTop() < distance ) {
        $(".site-header").removeClass("fixed");
    }
});


If one is true you don't need to check the other condition.

Same thing with the other set of code as well

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

    if (scroll >= 200) {
        $("body").addClass("scrolled");
    }
    if (scroll <= 200) {
        $("body").removeClass("scrolled");
    }
});


should be something like this

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

    if (scroll >= 200) {
        $("body").addClass("scrolled");
    } else if (scroll < 200) {
        $("body").removeClass("scrolled");
    }
});


You were actually performing both if statements if scroll = 200, which is probably not what you wanted so I changed the equality expression to reflect what I thought you wanted.

Both functions could have an if/else instead of an if/else if because if you don't meet the criteria for the first statement you meet the criteria for everything else with the else if.

I also removed an unnecessary variable in the second function as well

$window.scroll(function() {
    if ( $window.scrollTop() >= distance ) {
        $(".site-header").addClass("fixed");
    } else {
        $(".site-header").removeClass("fixed");
    }
});

$(window).scroll(function() {    
    if ($(window).scrollTop() >= 200) {
        $("body").addClass("scrolled");
    } else {
        $("body").removeClass("scrolled");
    }
});


Really what you are doing here is overloading that function, but you don't give it any options which function to run, so what would most likely happen is that both sets of functions will run.

I am not sure what the difference is between $(window).Scroll(function() ... and $window.Scroll(function()... is, maybe that is how you distinguish between which version of the function to run.

If you are going to run both code at the same time you could just put them together inside one function call.

Code Snippets

var distance = $('.site-header').offset().top,
    $window = $(window);

$window.scroll(function() {
    if ( $window.scrollTop() >= distance ) {
      $(".site-header").addClass("fixed");
    }
    if ( $window.scrollTop() < distance ) {
      $(".site-header").removeClass("fixed");
    }
});
$window.scroll(function() {
    if ( $window.scrollTop() >= distance ) {
        $(".site-header").addClass("fixed");
    } else if ( $window.scrollTop() < distance ) {
        $(".site-header").removeClass("fixed");
    }
});
$(window).scroll(function() {    
    var scroll = $(window).scrollTop();

    if (scroll >= 200) {
        $("body").addClass("scrolled");
    }
    if (scroll <= 200) {
        $("body").removeClass("scrolled");
    }
});
$(window).scroll(function() {    
    var scroll = $(window).scrollTop();

    if (scroll >= 200) {
        $("body").addClass("scrolled");
    } else if (scroll < 200) {
        $("body").removeClass("scrolled");
    }
});
$window.scroll(function() {
    if ( $window.scrollTop() >= distance ) {
        $(".site-header").addClass("fixed");
    } else {
        $(".site-header").removeClass("fixed");
    }
});

$(window).scroll(function() {    
    if ($(window).scrollTop() >= 200) {
        $("body").addClass("scrolled");
    } else {
        $("body").removeClass("scrolled");
    }
});

Context

StackExchange Code Review Q#63583, answer score: 5

Revisions (0)

No revisions yet.