3
\$\begingroup\$

I am using this code to get images to load only if they are visible. However, it seems to be slow with thousands of images, even if they are not rendered.

function getViewportHeight() {
    if(window.innerHeight) {
        return window.innerHeight;
    }
    else if(document.body && document.body.offsetHeight) {
        return document.body.offsetHeight;
    }
    else {
        return 0;
    }
}
function inView(elem, nearThreshold) {
    var viewportHeight = getViewportHeight();
    var scrollTop = (document.documentElement.scrollTop ? document.documentElement.scrollTop : document.body.scrollTop);
    var elemTop = elem.offset().top;
    var elemHeight = elem.height();
    nearThreshold = nearThreshold || 0;
    if((scrollTop + viewportHeight + nearThreshold) > (elemTop + elemHeight)) {
        return true;
    }
    return false;
}

function loadVisibleImages() {
    jQuery('img[data-src]').each(function() {
        if(jQuery(this).is(':visible')) {
            if(inView(jQuery(this), 100)) {
                this.src = jQuery(this).attr('data-src');
            }
        }
    });
}

jQuery(window).scroll(function() {
    loadVisibleImages();
});

I am using this code to render images:

<img src="" data-src="http://placehold.it/400x400" alt="" width="400" height="400">

Everything works, but how can I optimize it?

\$\endgroup\$
3
\$\begingroup\$

Here are a few callouts/suggestions:

You should throttle your code on the scroll event. That way the event doesn't fire thousands of times. You can read up this article for more information. A basic example would be:

//which makes the `scroll` event fire every 250ms.
$(window).scroll( $.throttle( 250, loadVisibleImages) );

You probably don't need to get the viewport size every time through the loop. This should be called once at the start and then only on the resize event. Otherwise, it never changes so there is no need to re-calculate it.

//which makes the `resize` event fire every 250ms.
$(window).resize( $.throttle( 250, getViewportHeight) );

You can add a class to all the images that have already been processed and exclude them from your selector. This way you are only dealing with new images. For example:

function loadVisibleImages() {
  jQuery('img[data-src]').not('.processed').each(function() {
    if(jQuery(this).is(':visible')) {
        if(inView(jQuery(this), 100)) {
            this.src = jQuery(this).attr('data-src').addClass('processed');
        }
    }
  });
}

If you haven't already you should wrap your code in an IIFE so you can create a private scope. Just take the code below and put it around your existing code. This would also allow you to use $ for jQuery without worry.

(function( $ ) {
  //your code here
}) ( jQuery );

You could optimize the loop in your loadVisibleImages functions. See your other question.

Hope that helps!

\$\endgroup\$
  • \$\begingroup\$ Very valid points with the resize and the throttle functions. Let me test the code and I'll get back to you. \$\endgroup\$ – Ciprian May 8 '15 at 14:57
  • \$\begingroup\$ I had to rewrite the function, as jQuery :visible is quite slow, so I decided to implement the function for each AJAX call. But the answer is correct and it speeds up the function a lot. \$\endgroup\$ – Ciprian May 8 '15 at 15:11
2
\$\begingroup\$

In method inView you recount variables which are the same for every scroll position:

viewportHeight, scrollTop,nearThreshold and their sum

You can count them once in the scroll event handler and then pass to inView only their sum.

\$\endgroup\$
2
\$\begingroup\$

I am not a fan of nesting statements more than you have to, I would suggest that you un-nest this piece of code a little bit to make it a little bit cleaner(in my opinion)

function loadVisibleImages() {
    jQuery('img[data-src]').each(function() {
        if(jQuery(this).is(':visible')) {
            if(inView(jQuery(this), 100)) {
                this.src = jQuery(this).attr('data-src');
            }
        }
    });
}

like this

function loadVisibleImages() {
    jQuery('img[data-src]').each(function() {
        if(jQuery(this).is(':visible') && inView(jQuery(this), 100)) {
            this.src = jQuery(this).attr('data-src');
        }
    });
}

It is essentially the same thing that you have, except for that it is not nested as deeply.


also inside of your inView function you could eliminate the if statement at the end, it would change from this

if((scrollTop + viewportHeight + nearThreshold) > (elemTop + elemHeight)) {
    return true;
}
return false;

to this

return (scrollTop + viewportHeight + nearThreshold) > (elemTop + elemHeight)
\$\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.