jQuery scroll-prevention plugin

Question

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 );

Answer 1

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.

• Greg Burghardt: 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.
• janos: @GregBurghardt these are not declarations, but modifications. The variables are already declared, the lines are pointed out modify them.
• Greg Burghardt: You are right. It was hard to tell which part of the OP's code you were referring to, so I got confused.
• janos: The part after the // Calculate dynamic values comment in the OP's code. I added the comment now to my post to avoid misunderstandings ;-)