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);
}
| improve this question | | | | |
\$\endgroup\$
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);
  });
| improve this answer | | | | |
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.