3
\$\begingroup\$

I recently was playing around with the idea of having an image appear when the user scrolls and came up with the following. Is there a better or more efficient way to do it?

var pastScroll;
jQuery(document).scroll(function () {
    var currentScroll = jQuery(document).scrollTop();
    if (pastScroll == currentScroll) {
        //console.log("not moving");
        jQuery('#content').css('background-image', 'url("/onScroll/blank.png")');
        // clear background
    }
    else {
        jQuery('#content').css('background-image', 'url("/onScroll/logo.png")');
        jQuery('#content').css('background-repeat','no-repeat');
        jQuery('#content').css('background-position', 'center'); 
        //console.log('moving');
        pastScroll = currentScroll;
        setTimeout(function () {
            if (pastScroll == currentScroll) {
                //console.log("not moving");
                jQuery('#content').css('background-image', 'url("/onScroll/blank.png")');
            }
        }, 100)
    }
});

You can ignore the use of jQuery instead of $ because it's being used in wordpress and ignore the css style added. I'm just wanting to focus on the conditionals and triggering. Any constructive criticism is appreciated, thanks.

\$\endgroup\$
  • \$\begingroup\$ Welcome to Code Review! I hope you get some great answers. \$\endgroup\$ – Phrancis Sep 1 '16 at 13:26
  • \$\begingroup\$ Thanks Phrancis, this community already appears more welcoming than some of the other on stackexchange. Hopefully I can help out some code reviews soon. \$\endgroup\$ – D. Cantatore Sep 1 '16 at 14:34
4
\$\begingroup\$
  1. Use CSS to change the background image of an element. It's easy and safe than using css() to change the styles of element from JavaScript. This also keep separation of concerns. Tomorrow, if you want to change the image size, you just have to go to the CSS and update the particular CSS class.

    Also, note that the three properties background-image, background-repeat and background-position can be combined into background property.

    CSS:

    .logo {
        background: url('/onScroll/logo.png') center no-repeat;
    }
    .blank {
        background: url('/onScroll/blank.png');
    }
    

    JavaScript:

    $('#content')
        .addClass('logo')       // Add class
        .removeClass('blank');  // Remove class
    
  2. Wrap the code in IIFE to avoid cluttering global namespace by creating global variables. Also, passing the variables to IIFE will create local alias reducing scope chain operations.

    (function ($, document) {
         // CODE HERE
         ...
    }(jQuery, document));
    

    Note that jQuery is passed as first parameter and catch-ed in $. Less typing. More readable.

  3. Cache the element references. By doing this, the elements are not needed to be taken from DOM again. Thus improving performance.

    // Caching
    var $document = $(document),
        $content = $('#content');
    
  4. Use strict equality operators for comparison. See Which equals operator (== vs ===) should be used in JavaScript comparisons?

    if (pastScroll === currentScroll) {
    
  5. Chaining. jQuery provides feature to chain multiple calls on same elements.

    jQuery('#content').css('background-image', 'url("/onScroll/logo.png")');
    jQuery('#content').css('background-repeat','no-repeat');
    jQuery('#content').css('background-position', 'center');
    

    Instead of this, multiple calls can be chained

    jQuery('#content').css('background-image', 'url("/onScroll/logo.png")')
        .css('background-repeat', 'no-repeat')
        .css('background-position', 'center');
    

    OR better, jQuery allows to pass an object to css() where multiple styles can be set in one call.

    jQuery('#content').css({
        'background-image': 'url("/onScroll/logo.png")',
        'background-repeat': 'no-repeat',
        'background-position': 'center'
    });
    

    Better way is to use CSS classes as explained in #1 above.

  6. Clearing previously set timeouts, if any.

    As setTimeout is used, if user keeps scrolling continuously the timeout callback will be executed multiple number of times. This is not really required. You can use clearTimeout to clear previously set timeouts, thus only one timeout will be executed once user has stopped scrolling and 100 ms is passed.

Complete Code:

CSS:

.logo {
    background: url('/onScroll/logo.png') center no-repeat;
}
.blank {
    background: url('/onScroll/blank.png');
}

JavaScript:

// Wrap the code in IIFE
(function ($, document) {
    'use strict';

    // $        = an alias for jQuery
    // document = alias for document object

    // Private variables
    var pastScroll,
        timeout;

    // Caching
    var $document = $(document),
        $content = $('#content');

    $document.scroll(function () {
        var currentScroll = $document.scrollTop();

        // Use strict equality operator
        if (pastScroll === currentScroll) {
            // For multiple method calls on same element, use chaining
            // Use CSS class to set the background image
            $content.addClass('blank').removeClass('logo');
        } else {
            $content.addClass('logo').removeClass('blank');

            pastScroll = currentScroll;

            // Clear previous timeouts
            clearTimeout(timeout);
            timeout = setTimeout(function () {
                if (pastScroll === currentScroll) {
                    $content.addClass('blank').removeClass('logo');
                }
            }, 100);
        }
    });
}(jQuery, document));
| improve this answer | |
\$\endgroup\$
  • 1
    \$\begingroup\$ It was not specifically mentioned in the answer above, but I wanted to point out how @Tushar changed your loose comparisons (==) to exact comparisons (===). This is generally preferred coding behavior. I personally always default to using exact comparisons and only use loose comparisons when a specific situation warrants it. This approach will make your code less fragile and easier to debug. \$\endgroup\$ – Mike Brant Sep 1 '16 at 14:15
  • \$\begingroup\$ Thanks Mike, I'm aware of matching variable type and leaving no room for coercion. Bad habit of using 2 that I've got to stop doing \$\endgroup\$ – D. Cantatore Sep 1 '16 at 14:22
0
\$\begingroup\$

All excellent points @Tushar. Always cache variables for your code. Using the selector, $(".foo"), always goes back to the DOM for the value and only in certain examples is needed. A good example of that would be needing to know how many LI elements come back in from an Ajax call. In that case the $(".foo") is needed to update the count because the cached selector would always have the previous value.

Lastly, using the .scroll() is always heavy and using the setTimeout is a great way to prevent recursive overload. Because I started using timeout so much I stumbled upon a nice little function that ensures a one time execution.

var taskDone = (function(){
    var timers = {};
    return function(cal, mili, uniqueId){
        if(timers[uniqueId]){
            clearTimeout(timers[uniqueId]);
        };
        timers[uniqueId] = setTimeout(cal, mili);
    };
})(); //usage taskDone(function(){}, 500, "someUniqueString");
| 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.