4
\$\begingroup\$

This plugin is for article pages that has a very long main content (left side) and a shorter sidebar. When the user scrolls down mostly the right part will be white space.

What this plugin does is make the sidebar stick or make it fixed past a point and will stop sticking when it collides with the bottom part or a footer. The functionality will not work if the main content is short.

Anyway, I'm asking feedback on how to improve my JS code.

;(function ( $, window, document, undefined ) {

// Create the defaults once
var pluginName = "scrollMinUntil",
defaults = {
    article: null,
    topPadding: 0
};

// The actual plugin constructor
var Plugin = function ( element, options ) {
this.element = element;
this.options = $.extend( {}, defaults, options);
this._defaults = defaults;
this._name = pluginName;
this.init();
}

Plugin.prototype = {

init: function() {

    if($(this.element).length <= 0)
      return null;

    this.scroll(this.element, this.options);            
},

scroll: function(el, options) {
    var article = options.article,
        topPadding = options.topPadding,
        w = $(window),
        wrap = $(el).children('.wrap'),
        startingPos = wrap.offset().top,
        fixedPosTrigger = wrap.offset().top + wrap.outerHeight(),
        stopMovingPosTrigger = 0,
        staticMarginTop = 0,
        scrollTop = 0,
        elHeight = 0,
        stopMovingPos = 0,
        fixed = false,
        staticFlag = false;

    if(options.article.outerHeight() > 1500) {
      w.scroll(function() {

        // Calculate dynamic values
        scrollTop = $(this).scrollTop(),
        elHeight = wrap.outerHeight(),
        fixed = wrap.hasClass('fixed'),
        staticFlag = wrap.hasClass('static'),
        stopMovingPos = article.outerHeight() + article.offset().top;

        // Check if scrolling before or after the fixed trigger, set stopMovingPosTrigger accordingly
        (scrollTop < fixedPosTrigger) ? stopMovingPosTrigger = scrollTop : stopMovingPosTrigger = scrollTop + elHeight;

        // Making sure that fixed is triggered when scrolling pass the trigger position from the original area
        if(scrollTop > fixedPosTrigger && !staticFlag)
          wrap
            .addClass('fixed')
            .css({
              'padding-top': topPadding + 'px'
            });

        // Fixed positioned area, setting paddings and margins, and transition scrolling from the static area
        if((scrollTop > fixedPosTrigger) && (stopMovingPosTrigger < stopMovingPos)){
          wrap
            .removeClass('static')
            .addClass('fixed')
            .css({
              'margin-top': 0
            });
        }

        // Static positioned area, setting dynamically the margin-top for the element
        else if((stopMovingPosTrigger > stopMovingPos) && fixed) {

          // Calculate top margin to be added to the static element
          staticMarginTop = (stopMovingPos - startingPos) - elHeight;

          wrap
            .removeClass('fixed')
            .addClass('static')
            .css({
              'margin-top': staticMarginTop
            });
        }

        // Original area before the fixed position area
        else if(scrollTop < fixedPosTrigger) {
          wrap
            .removeClass('fixed static')
            .css({
              'padding-top': 0
            });
        }
      });
    }
}
};

// A really lightweight plugin wrapper around the constructor,
// preventing against multiple instantiations
$.fn[pluginName] = function ( options ) {
return this.each(function () {
    if (!$.data(this, "plugin_" + pluginName)) {
        $.data(this, "plugin_" + pluginName,
        new Plugin( this, options ));
    }
});
};

})( jQuery, window, document );
\$\endgroup\$
1
\$\begingroup\$

This part is pretty odd:

    // Calculate dynamic values
    scrollTop = $(this).scrollTop(),
    elHeight = wrap.outerHeight(),
    fixed = wrap.hasClass('fixed'),
    staticFlag = wrap.hasClass('static'),
    stopMovingPos = article.outerHeight() + article.offset().top;

The 5 assignments are separated by commas, instead of semi-colons. It would be better to end each of the first 4 lines with a semi-colon.

This is even worse, using assignment in the right-hand sides of a ternary operator:

    (scrollTop < fixedPosTrigger) ? stopMovingPosTrigger = scrollTop : stopMovingPosTrigger = scrollTop + elHeight;

It would be shorter and better to move stopMovingPosTrigger to the left, and use the ternary at the right-hand side of the assignment:

    stopMovingPosTrigger = scrollTop < fixedPosTrigger ? scrollTop : scrollTop + elHeight;

Inside the scroll function, these conditions and their negated values appear multiple times:

  • scrollTop > fixedPosTrigger
  • stopMovingPosTrigger > stopMovingPos

To reduce the duplication which may lead to errors, I suggest giving these conditions a name by storing them in variables, like this:

var fixedPositionArea = scrollTop > fixedPosTrigger;
var staticPositionArea = stopMovingPosTrigger > stopMovingPos;

This way, instead of re-typing scrollTop > fixedPosTrigger multiple times, you could use fixedPositionArea and !fixedPositionArea in your conditions. Of course, !fixedPositionArea is not exactly the same as scrollTop < fixedPosTrigger, but it seems to me this small discrepancy won't break your script.

Using these variables the code becomes:

if (fixedPositionArea && !staticFlag) {
    wrap
        .addClass('fixed')
        .css({ 'padding-top': topPadding + 'px' });
}

// Fixed positioned area, setting paddings and margins, and transition scrolling from the static area
if (fixedPositionArea && !staticPositionArea) {
    wrap
        .removeClass('static')
        .addClass('fixed')
        .css({ 'margin-top': 0 });
}

// Static positioned area, setting dynamically the margin-top for the element
else if (staticPositionArea && fixed) {
    // Calculate top margin to be added to the static element
    staticMarginTop = stopMovingPos - startingPos - elHeight;

    wrap
        .removeClass('fixed')
        .addClass('static')
        .css({ 'margin-top': staticMarginTop });
}

// Original area before the fixed position area
else if (!fixedPositionArea) {
    wrap
        .removeClass('fixed static')
        .css({ 'padding-top': 0 });
}

Some blocks are not indented well, for example:

var Plugin = function ( element, options ) {
this.element = element;
this.options = $.extend( {}, defaults, options);
this._defaults = defaults;
this._name = pluginName;
this.init();
}

It would be better to indent the body inside a { ... } block, like this:

var Plugin = function ( element, options ) {
    this.element = element;
    this.options = $.extend( {}, defaults, options);
    this._defaults = defaults;
    this._name = pluginName;
    this.init();
};

Also, since this is an assignment, the right-hand side should be terminted with a semi-colon.

Review the rest of the code and correct all the under-indented blocks.

It would be also good to use consistent amount of indents. In most places you seem to use 4 spaces, while in others only 2. I suggest to pick one width and use it consistently everywhere.

|improve this answer|||||
\$\endgroup\$
  • \$\begingroup\$ RE: separating variable declarations with a comma: This is usually done to save a little on file size, since your won't need multiple var directives. I have a love-hate relationship with this. I like seeing var because it's a good visual queue that a bunch of variables are being declared, but I do like the (miniscule) file size savings. \$\endgroup\$ – Greg Burghardt Dec 17 '14 at 15:18
  • \$\begingroup\$ @GregBurghardt these are not declarations, but modifications. The variables are already declared, the lines are pointed out modify them. \$\endgroup\$ – janos Dec 17 '14 at 15:45
  • \$\begingroup\$ You are right. It was hard to tell which part of the OP's code you were referring to, so I got confused. \$\endgroup\$ – Greg Burghardt Dec 17 '14 at 15:57
  • \$\begingroup\$ The part after the // Calculate dynamic values comment in the OP's code. I added the comment now to my post to avoid misunderstandings ;-) \$\endgroup\$ – janos Dec 17 '14 at 15:59

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.