# jQuery scroll-prevention plugin

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 );  ## 1 Answer 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.

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