patternjavascriptMinor
Multiple uses of $window.scroll
Viewed 0 times
windowusesscrollmultiple
Problem
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() = 200) {
$("body").addClass("scrolled");
}
if (scroll <= 200) {
$("body").removeClass("scrolled");
}
});
});Solution
You have an extra if statement in these that should be structured as an if/else statement, or even an if/else if statement
You should write it like this instead:
If one is true you don't need to check the other condition.
Same thing with the other set of code as well
should be something like this
You were actually performing both if statements if
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
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
If you are going to run both code at the same time you could just put them together inside one function call.
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.
Code Snippets
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");
}
});$window.scroll(function() {
if ( $window.scrollTop() >= distance ) {
$(".site-header").addClass("fixed");
} else if ( $window.scrollTop() < distance ) {
$(".site-header").removeClass("fixed");
}
});$(window).scroll(function() {
var scroll = $(window).scrollTop();
if (scroll >= 200) {
$("body").addClass("scrolled");
}
if (scroll <= 200) {
$("body").removeClass("scrolled");
}
});$(window).scroll(function() {
var scroll = $(window).scrollTop();
if (scroll >= 200) {
$("body").addClass("scrolled");
} else if (scroll < 200) {
$("body").removeClass("scrolled");
}
});$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");
}
});Context
StackExchange Code Review Q#63583, answer score: 5
Revisions (0)
No revisions yet.