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");           
        }
    });
});
| improve this question | | | | |
\$\endgroup\$
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);
    });
});
| improve this answer | | | | |
\$\endgroup\$
  • 1
    \$\begingroup\$ very nice answer! \$\endgroup\$ – Dan Cantir Feb 21 '17 at 13:07
  • \$\begingroup\$ Thank you. Your answer is really good. Just some tiny bits you left behind. \$\endgroup\$ – Ismael Miguel Feb 21 '17 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\$ – theonlygusti Feb 21 '17 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\$ – Ismael Miguel Feb 21 '17 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\$ – TheCrazyProfessor Feb 21 '17 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);
  });

});
| improve this answer | | | | |
\$\endgroup\$
  • \$\begingroup\$ that was a much better way :o \$\endgroup\$ – TheCrazyProfessor Feb 21 '17 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\$ – TheCrazyProfessor Feb 21 '17 at 9:26
  • \$\begingroup\$ @TheCrazyProfessor, I've updated my answer, could you please check new version? \$\endgroup\$ – Dan Cantir Feb 21 '17 at 9:29
  • \$\begingroup\$ Amazing! now it works \$\endgroup\$ – TheCrazyProfessor Feb 21 '17 at 9:31
  • 1
    \$\begingroup\$ The cleanest if statement is the if statement that isn't there :) \$\endgroup\$ – theonlygusti Feb 21 '17 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.

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