jQuery navbar scrollTop position

Question

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"); 
 }
 });
});

Answer 1

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);
 });
});

• Dan Cantir: very nice answer!
• Ismael Miguel: Thank you. Your answer is really good. Just some tiny bits you left behind.
• user98809: 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.
• Ismael Miguel: @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.
• TheCrazyProfessor: 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

Answer 2

I would write this like:

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

• TheCrazyProfessor: that was a much better way :o
• TheCrazyProfessor: but for reason if I use toggleClass it can't find out what to do, so the class keep toggle on and off
• Dan Cantir: @TheCrazyProfessor, I've updated my answer, could you please check new version?
• TheCrazyProfessor: Amazing! now it works
• user98809: The cleanest if statement is the if statement that isn't there :)

Answer 3

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.