7
\$\begingroup\$

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() < distance ) {
        $(".site-header").removeClass("fixed");
    }
});

// Add class .scrolled on scroll to control show/hide of back-top button

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

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

});
\$\endgroup\$
8
  • \$\begingroup\$ Have you tested this code? does it function how you anticipated it would? \$\endgroup\$
    – Malachi
    Sep 22, 2014 at 16:25
  • 1
    \$\begingroup\$ Yeah, it's working perfectly. I'm just looking to see if I'm approaching it in a good way. \$\endgroup\$ Sep 22, 2014 at 18:55
  • 1
    \$\begingroup\$ Not as yet. How would I test an exact value like that? I've switched the code to scroll < 200 for the second part of the second function to avoid both firing at exactly 200. \$\endgroup\$ Sep 22, 2014 at 19:03
  • 1
    \$\begingroup\$ that should work then, I figured you didn't want both firing at the same time, you would notice a glitch when you hit a value of 200 when testing the page I am sure. \$\endgroup\$
    – Malachi
    Sep 22, 2014 at 19:11
  • 1
    \$\begingroup\$ var scroll = 200; instead of var scroll = $(window).scrollTop(); . also make sure that you are testing in more than just Chrome. at least IE 8 to IE 11, Safari, and Firefox \$\endgroup\$
    – Malachi
    Sep 23, 2014 at 13:08

4 Answers 4

5
\$\begingroup\$

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.

\$\endgroup\$
1
  • 1
    \$\begingroup\$ Regarding if/else the second option is more correct. Also, I think giving meaningful names to the anonymous function would make the code readable and intentions clear. \$\endgroup\$
    – CodeYogi
    Apr 15, 2016 at 18:03
2
\$\begingroup\$

It certainly looks like you could combine this into one function. My JS is a bit rusty, but something like this should work:

var onScroll = function(minimumDistance, className){
    var scroll = $(window).scrollTop();

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

I'd recommend a better function name, but I'm drawing a blank.

I've also replaced the <= on the second if clause, because if scroll was exactly equal to minimumDistance then you'd just add the class and immediately remove it.

As to whether you "should" - I'm a big fan of going in DRY when coding, because it usually saves me time in the long run when I'm fixing bugs or refactoring. This is only duplicated once, so you might not agree or may even find it a case of overenginerring, but if there's a fair chance you'll want to use it anywhere else (within or outside this project), or you'll be supporting / refactoring this code, then I'd recommend abstracting it.

\$\endgroup\$
5
  • \$\begingroup\$ Thanks. I'll be likely to use both seperate functions seperately on other projects, so might be worth keeping them seperate. What do you mean by "abstracting" it? (As in, exactly what are you suggesting I abstract?) \$\endgroup\$ Sep 22, 2014 at 15:13
  • 1
    \$\begingroup\$ My code snippet above is an abstraction of your code. It takes the essence of your two functions (i.e. deciding on adding or removing a class based on scroll distance) and separates that from the exact values (scroll distance and the class to be added/removed). This means the code can be reused no matter what class or scroll distance you want, and should you later decide to change class or scroll distance, you only have to alter the parameters. On top of that you only need to debug one place, not n places (where n is the number of times you write a function like that). \$\endgroup\$
    – Nick Udell
    Sep 22, 2014 at 15:18
  • \$\begingroup\$ If, as you say, you plan on using both separate functions separately on other projects, then definitely use the abstraction with different parameters. Modifications will only need to be made to one place, and it saves you accidentally fixing a bug in one implementation but forgetting to do so in another one, leading to inconsistent functionality. \$\endgroup\$
    – Nick Udell
    Sep 22, 2014 at 15:18
  • \$\begingroup\$ Thanks that's a really helpful reply! I'll vote your answer up when I get enough reputation here. \$\endgroup\$ Sep 22, 2014 at 15:23
  • \$\begingroup\$ you should add an element parameter as well, then it really could be used for either function in the OP \$\endgroup\$
    – Malachi
    Mar 30, 2016 at 12:47
2
\$\begingroup\$

A more idiomatic way to add and remove classes in jQuery is .toggleClass(). You can write one declaration instead of an if-else.

It's probably a good idea to combine the two handlers. However, there may be good reasons for keeping them separate, if it makes more sense for your organization (for example, if the affected UI elements are maintained by separate developers).

The distance variable could use a more descriptive name.

$(function() {
    var siteHeaderTop = $('.site-header').offset().top;
    var $window = $(window);

    $window.scroll(function() {
        var windowTop = $window.scrollTop();
        $('.site-header').toggleClass('fixed', windowTop >= siteHeaderTop);

        // Add class .scrolled on scroll to control show/hide of back-top button
        $('body').toggleClass('scrolled', windowTop > 200);
    });
});
.site-header {
    position: relative;
    top: 100px;
    border: 1px solid black;
    width: 20em;
}
.site-header.fixed {
    position: fixed;
    top: 1px;
}

div {
    display: none;
}
body.scrolled div {
    display: block;
    position: fixed;
    bottom: 0;
    right: 10px;
}
<!DOCTYPE html>
<html>
  <head>
    <meta charset="UTF-8">
    <title>Scroll demo</title>
    <script src="https://ajax.googleapis.com/ajax/libs/jquery/1.11.1/jquery.min.js"></script>
  </head>
  <body id="top" style="height: 8000px;">
    <nav class="site-header">Site header</nav>
    <div><a href="#top">ꜛ Back to top</a></div>
  </body>
</html>


Since $window.scrollTop() is a global property, it may make more sense to set both of the classes on the <body> element instead.

$(function() {
    var siteHeaderTop = $('.site-header').offset().top;
    var $window = $(window);

    $window.scroll(function() {
        var windowTop = $window.scrollTop();
        $('body').toggleClass('scrolled-past-site-header', windowTop > siteHeaderTop)
                 .toggleClass('scrolled-past-200', windowTop > 200);
    });
});
.site-header {
    position: relative;
    top: 100px;
    border: 1px solid black;
    width: 20em;
}
body.scrolled-past-site-header .site-header {
    position: fixed;
    top: 1px;
}

div {
    display: none;
}
body.scrolled-past-200 div {
    display: block;
    position: fixed;
    bottom: 0;
    right: 10px;
}
<!DOCTYPE html>
<html>
  <head>
    <meta charset="UTF-8">
    <title>Scroll demo</title>
    <script src="https://ajax.googleapis.com/ajax/libs/jquery/1.11.1/jquery.min.js"></script>
  </head>
  <body id="top" style="height: 8000px;">
    <nav class="site-header">Site header</nav>
    <div><a href="#top">ꜛ Back to top</a></div>
  </body>
</html>

\$\endgroup\$
2
  • \$\begingroup\$ How did you relate to the global property to the body element seems a cool idea to me. Is there any related text to explore this kind of idea further? One more thing, why hard coding 200 in class name? what is we decide to change it to 400? change in three places. \$\endgroup\$
    – CodeYogi
    Apr 15, 2016 at 18:09
  • \$\begingroup\$ @CodeYogi If I had to give a name to this idea, I would say that it's just using the Cascading principle of CSS. The 200 is only hard-coded in one place, if you consider that scrolled-past-200 is just a silly example of a class name that should be replaced with something more meaningful, like scrolled-past-site-header. \$\endgroup\$ Apr 15, 2016 at 19:59
0
\$\begingroup\$

Try this ;)

$(function(){
  var distance = $('.site-header').offset().top;

  /* cached jQuery objects for better performance */
  var $window = $(window);
  var $siteHeader = $(".site-header");
  var $body = $("body");

  $window.scroll(function(){
    /* we can use same scrollTop for both the conditions */
    var scrollTop = $window.scrollTop();

    /* Add class .fixed when .site-header hits top of viewport */
    $siteHeader.toggleClass("fixed", (scrollTop >= distance));

    /* Add class .scrolled on scroll to control show/hide of back-top button */
    $body.toggleClass("scrolled", (scrollTop >= 200));
  });
});

We should cache jQuery objects we are referencing too may times as it reduces the DOM traversal time to find that element every time we want to do something with it; And Here 2nd if condition not required as if this will be true or false so we can use if(){...}else{...} instead;

And the values we are referencing multiple times we can store them in variable and can reference them here $window.scrollTop() value;

$(selector).toggleClass(className [, state]);

className Type: String

One or more class names (separated by spaces) to be toggled for each element in the matched set.

state Type: Boolean

A Boolean (not just truthy/falsy) value to determine whether the class should be added or removed.

So as per the api documentation we can use this function to change class add or remove with a state parameter to determine that the:

class should be added if passed true for state and

remove if state is false;

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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