2
\$\begingroup\$

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);
}
Jamal
35.2k13 gold badges134 silver badges238 bronze badges
asked Sep 25, 2013 at 20:07
\$\endgroup\$

1 Answer 1

2
\$\begingroup\$

I'd like to address a couple of style concerns over simplification.

  1. 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).

  2. 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.

  3. 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);
 });
answered Sep 26, 2013 at 2:32
\$\endgroup\$

Your Answer

Draft saved
Draft discarded

Sign up or log in

Sign up using Google
Sign up using Email and Password

Post as a guest

Required, but never shown

Post as a guest

Required, but never shown

By clicking "Post Your Answer", you agree to our terms of service and acknowledge you have read our privacy policy.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.