6
\$\begingroup\$

I have this code, but I not the best to javascript/jQuery so are there any out there who can optimize this code?

$(function(){
    var pos = $("header").offset().top;

    $(document).scroll(function(){
        if($(this).scrollTop() - 10 > pos)
        {   
           $('header').addClass("filled");
        } else {
           $('header').removeClass("filled");           
        }
    });
});
\$\endgroup\$

3 Answers 3

7
\$\begingroup\$

I have a few questions to ask about your code, before I dive in:

  • Why are you storing the header's location?
  • Isn't the header supposed to be on the top?
  • If it is at the top, isn't it at position 0?
  • If not, are you sure you don't have some funky CSS sending weird margins and paddings over the top of the page?

Alright, let's dig into the code!

From now on, I will assume that the header has a weird position.


While you're doing right in using (the equivalent of) $(document).ready(), you are forgetting that jQuery.noConflict(); can be called, breaking your code.

My suggestion is to use something like this:

window.jQuery(function($){
    [...]
});

The window.jQuery() (or $()) allows you to create an alias to the jQuery object, being it passed as the first parameter.
You can read about it on https://api.jquery.com/ready/


As @Dan said, you are re-re-re-re-re-forcing jQuery to painfully look for the header, every time you scroll. Can you imagine how slow that is!?

I will take @Dan's suggestion and change it a bit:

var $header = $('header');
var threshold = 10;
var offsetTop = $header.offset().top - threshold;

Why I think this is better:

  • The variable names have a better meaning ($header -> $('header')).
  • You can re-use the $header, without having to look for it all the time.
  • You are already storing the value you will need later on, without you having to subtract threshold.
  • You eliminated a magic number!

Taking on from @Dan's suggestion, they have the following piece of code:

$(document).scroll(function(){
    $header.toggleClass('filled', $(this).scrollTop() - 10 > threshold);
});

This is a reasonable improvement, but it is still flawed.

I propose the following:

var $document = $(document).scroll(function(){
    $header.toggleClass('filled', $document.scrollTop() > offsetTop);
});

This stores and re-used the $document created, instead of always running a useless $(this). And since I've already subtracted the magic number, there's no need to re-calculate it here.


I always suggest wrapping the code on a ;(function(window, undefined){ [...] })(Function('return this')());, but I leave it for you to decide if it is worth it or not.


And now, all together:

window.jQuery(function($){
    var $header = $('header');
    var threshold = 10;
    var offsetTop = $header.offset().top - threshold;

    var $document = $(document).scroll(function(){
        $header.toggleClass('filled', $document.scrollTop() > offsetTop);
    });
});
\$\endgroup\$
12
  • 1
    \$\begingroup\$ very nice answer! \$\endgroup\$
    – Dan Cantir
    Feb 21, 2017 at 13:07
  • \$\begingroup\$ Thank you. Your answer is really good. Just some tiny bits you left behind. \$\endgroup\$ Feb 21, 2017 at 13:23
  • 1
    \$\begingroup\$ This is a really good answer, +1. But I think you should be using camelCase instead of underscores for your variables, as most JavaScript style guides recommend. \$\endgroup\$
    – minseong
    Feb 21, 2017 at 13:35
  • \$\begingroup\$ @theonlygusti You're right. Thanks for the reminder. I really don't like it, but it is the style guidelines and I should follow them. I've edited my answer to match your tip. \$\endgroup\$ Feb 21, 2017 at 14:07
  • \$\begingroup\$ Amazing answer and also you @Dan thank you all lot! I really wanna give you both a "accepted answer" but I can't so... this time I give it to Ismael. Thanks for two amazing answer \$\endgroup\$ Feb 21, 2017 at 14:29
9
\$\begingroup\$

I would write this like:

$(function(){

  var header = $('header');
  var threshold = header.offset().top;

  $(document).scroll(function(){
      header.toggleClass('filled', $(this).scrollTop() - 10 > threshold);
  });

});
\$\endgroup\$
6
  • \$\begingroup\$ that was a much better way :o \$\endgroup\$ Feb 21, 2017 at 9:11
  • \$\begingroup\$ but for reason if I use toggleClass it can't find out what to do, so the class keep toggle on and off \$\endgroup\$ Feb 21, 2017 at 9:26
  • \$\begingroup\$ @TheCrazyProfessor, I've updated my answer, could you please check new version? \$\endgroup\$
    – Dan Cantir
    Feb 21, 2017 at 9:29
  • \$\begingroup\$ Amazing! now it works \$\endgroup\$ Feb 21, 2017 at 9:31
  • 1
    \$\begingroup\$ The cleanest if statement is the if statement that isn't there :) \$\endgroup\$
    – minseong
    Feb 21, 2017 at 13:40
3
\$\begingroup\$

I'm continueing on the answer by Dan as this might still be buggy on lower powered devices.

I suggest you use a dethrottle/debounce technique. In some browsers, the scroll event is fired each scrolled pixel! This might kill performance. The before mentioned technique limits the event to every x milliseconds, dropping the need for performance:

// This example will trigger the event every 25ms.
$(document).scroll( $.throttle( 25, function(){
    header.toggleClass('filled', $(this).scrollTop() - 10 > threshold);
}))

In these cases I also like to add a small extra check to avoid the bigger functions. The toggleClass() functions isn't very heavy, but I thought it might be worth to demo:

var hasPassedThreshold = false;
$(document).scroll( $.throttle( 25, function(){
    var scrolledPastThreshold = $(this).scrollTop() - 10 > threshold;

    if( !hasPassedThreshold && scrolledPastThreshold ){
        header.addClass('filled');
    }else if( hasPassedThreshold && !scrolledPastThreshold ){
        header.removeClass('filled');
    }
    hasPassedThreshold = scrolledPastThreshold ;
}));

In this example it might give a small performance boost, but in case you start adding more functionallity on the if/else, this will lighten the load.

\$\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.