jQuery navbar scrollTop position

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


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

• very nice answer! – Dan Cantir Feb 21 '17 at 13:07
• Thank you. Your answer is really good. Just some tiny bits you left behind. – Ismael Miguel Feb 21 '17 at 13:23
• 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. – theonlygusti Feb 21 '17 at 13:35
• @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. – Ismael Miguel Feb 21 '17 at 14:07
• 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 – TheCrazyProfessor Feb 21 '17 at 14:29

I would write this like:

$(function(){ var header =$('header');

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

});

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

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 ){