I have an element where the jScrollPane
is being used to provide custom scrollbars for an extensive text. The idea behind the code is to change the image beside the text each time the user reaches a certain amount of scroll while reading the text.
To have the code flexible, I've prepared a method to detect the number of images available and from there establish the trigger points for the image change. This is all based on the text wrapper height.
Can't this be simplified, thus reducing the amount of code, perhaps in such reduction, have it improved?
HTML structure
<div id="imageWrap">
<img data-index="1" src="..." class="active" />
<img data-index="2" src="..." />
<img data-index="3" src="..." />
</div>
jQuery/JavaScript code
$imagesWrap = $('#imageWrap');
if ($imagesWrap.size()==1) {
//target all images and count them
var $targets = $imagesWrap.find('> img'),
total = $targets.size();
//for each found, set the CSS z-index
$targets.each(function(){
var $this = $(this),
index = $this.data('index');
//revert order
$this.css({
'z-index': total-index
});
});
var $scrollSpy = $('.jspPane'), //the element to spy
height = $scrollSpy.height(), //its height
stepsTrigger = parseInt(height/total), //trigger at "every" this distance
currentPos = 0; //current position (active image)
setInterval(function(){
//ascertain current position (insure positive number)
currentPos = Math.abs(parseInt($scrollSpy.css('top'), 10));
//ascertain the target image
var img = Math.round(currentPos/stepsTrigger),
id = img==0 ? 1 : img,
$tgt = $('img[data-index="'+id+'"]');
//if different from the already active
if (!$tgt.hasClass('active')) {
$imagesWrap.find('.active').removeClass('active').hide();
$tgt.fadeIn().addClass('active');
}
}, 100);
}
1 Answer 1
I'd like to address a couple of style concerns over simplification.
Purely for debugging if you have function expressions then name them.
eg. setInterval(function(){
could become
setInterval(function _scrollDetectionInterval(){
When debugging the call stack will now show you at a glance what this function is (especially helpful if you have multiple).
you have define variables outside of the scope you are using them.
var $scrollSpy = $('.jspPane'), //the element to spy height = $scrollSpy.height(), //its height stepsTrigger = parseInt(height/total), //trigger at "every" this distance currentPos = 0; //current position (active image)
This doesn't make sense unless you aren't going to change them again or you will be using them else where. Assuming this is all the code here I would at least put the
currentPos
inside the interval function scope.you have a
data-index="1"
attribute on your elements. Is this actually needed? jQuery has an index you can use in the .each() method. just change your function to$targets.each(function(index){
unless I am missing something it should work. You can also then change the line
$tgt = $('img[data-index="'+id+'"]');
to
$tgt = $targets.eq(Math.max(0, img - 1));
there is a lot going on in this line.
$targets.eq(
uses the 0-based index of the element in the array.Math.max(
ensures it will never be less than 0.
The code will only work if there is one image wrapper on the page. If you are looking to extend this to multiple here is how i would change it:
$('.someImageWrapClass')
.each(function _eachWrapper()
{
var $parentWrapper = $(this);
//target all images and count them
var $targets = $parentWrapper.find('> img'),
total = $targets.size();
//for each found, set the CSS z-index
$targets.each(function _eachImage(index){
//revert order
$(this).css({
'z-index': total - index
});
});
var $scrollSpy = $('.jspPane'), //the element to spy
height = $scrollSpy.height(), //its height
stepsTrigger = parseInt(height/total); //trigger at "every" this distance
setInterval(function _scrollDetection()
{
//current position (active image)
//ascertain current position (ensure positive number)
var currentPos = Math.abs(parseInt($scrollSpy.css('top'), 10));
//ascertain the target image
var img = Math.round(currentPos/stepsTrigger)
$tgt = $targets.eq(Math.max(0, img -1));
//if different from the already active
if (!$tgt.hasClass('active'))
{
$targets.removeClass('active').hide();
$tgt.fadeIn().addClass('active');
}
}, 100);
});