Reputation: 47
Is there a better way to do this to reduce code duplication?
Maybe somehow loop the 'pagesTop' array?
The following function is initialized on the windows scroll event.
Thanks.
function redrawSideNav() {
var pagesTop = new Array();
$('.page').each(function(index, elem){
pagesTop[index] = $(this);
});
$('.menu, .page').find('a').removeClass('active');
if ( $(document).scrollTop() >= pagesTop[0].offset().top && $(document).scrollTop() < pagesTop[1].offset().top) {
var target = '#' + pagesTop[0].attr('id');
$('[href^="'+target+'"]').addClass('active');
} else if ( $(document).scrollTop() >= pagesTop[1].offset().top && $(document).scrollTop() < pagesTop[2].offset().top) {
var target = '#' + pagesTop[1].attr('id');
$('[href^="'+target+'"]').addClass('active');
} else if ( $(document).scrollTop() >= pagesTop[2].offset().top && $(document).scrollTop() < pagesTop[3].offset().top) {
var target = '#' + pagesTop[2].attr('id');
$('[href^="'+target+'"]').addClass('active');
} else if ( $(document).scrollTop() >= pagesTop[3].offset().top && $(document).scrollTop() < pagesTop[4].offset().top) {
var target = '#' + pagesTop[3].attr('id');
$('[href^="'+target+'"]').addClass('active');
} else if ( $(document).scrollTop() >= pagesTop[4].offset().top) {
var target = '#' + pagesTop[4].attr('id');
$('[href^="'+target+'"]').addClass('active');
}
}
Upvotes: 2
Views: 149
Reputation: 4440
the code below works for dynamic sized arrays:
var scrollTop = $(document).scrollTop();
for (var i=0;i<pagesTop.length ;i++) {
tempCond = pagesTop[i+1] ? (scrollTop <= pagesTop[i+1].offset().top) : true;
if(scrollTop >= pagesTop[i].offset().top && tempCond ) {
var target = '#' + pagesTop[i].attr('id');
$('[href^="'+target+'"]').addClass('active');
break;
}
}
difference with samurai answer:
rather than adding a fictional item to the array to guarantee the loop is executed at least one time and the second condition of if statement is always true,
I check inside the loop if the current element is the last item in the array: if it is , the second condition in the if statement is directly set to true
, else, the condition is set to scrollTop <= pagesTop[i+1].offset().top
Upvotes: 1
Reputation: 20770
Well, it seems that only the number changes in the following code, so you can use a for-loop with a break-statement. When the break get's triggered, the rest of the for-loop will not be executed. The last else if(...)
is the same, but without the last condition. We can fake this by appending a dummy-element to pagesTop, that makes the last condition always true.
var a = {
offset: function() {
return {'top': 9999999999999};
}
};
pagesTop.push( a );
for( var i = 0; i < pagesTop.length-1; i++ ) {
if ( $(document).scrollTop() >= pagesTop[i].offset().top && $(document).scrollTop() < pagesTop[i+1].offset().top) {
var target = '#' + pagesTop[i].attr('id');
$('[href^="'+target+'"]').addClass('active');
break;
}
}
Upvotes: 1
Reputation: 49612
Do it step by step:
First I would create a variable for $(document).scrollTop()
and replace the code inside the if
statements:
var scrollTop = $(document).scrollTop();
var targetIndex = null;
$('.menu, .page').find('a').removeClass('active');
if ( scrollTop >= pagesTop[0].offset().top && scrollTop < pagesTop[1].offset().top) {
targetIndex = 0
} else if ( scrollTop >= pagesTop[1].offset().top && scrollTop < pagesTop[2].offset().top) {
targetIndex = 1;
} else if ( scrollTop >= pagesTop[2].offset().top && scrollTop < pagesTop[3].offset().top) {
targetIndex = 2;
} else if ( scrollTop >= pagesTop[3].offset().top && scrollTop < pagesTop[4].offset().top) {
targetIndex = 3;
} else if ( scrollTop >= pagesTop[4].offset().top) {
targetIndex = 4;
}
var target = '#' + pagesTop[targetIndex].attr('id');
$('[href^="'+target+'"]').addClass('active');
After that I would start building a loop instead if of using else if
cascades:
var scrollTop = $(document).scrollTop();
var targetIndex = null;
for (var i = 0; i < 4 && !targetindex; i++) {
if (scrollTop >= pagesTop[i].offset().top && scrollTop < pagesTop[i + 1].offset().top) {
targetIndex = i;
}
}
targetIndex = targetIndex || 4;
var target = '#' + pagesTop[targetIndex].attr('id');
$('[href^="'+target+'"]').addClass('active');
(untested.. maybe I made some indices/syntax errors..)
Upvotes: 1