HiveBrain v1.2.0
Get Started
← Back to all entries
patternjavascriptMinor

jQuery carousel plugin

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
pluginjquerycarousel

Problem

I have recently written my first jQuery plugin. It is a responsive jQuery carousel plugin. It seems to be functioning well apart from on window re-size. Sometimes if the window is re-sized too fast I get the following console error RangeError: Maximum call stack size exceeded, despite a timer function being used to prevent continuous re-rendering.

I'm looking for any best practices on things I've missed and should be following.

jsFiddle

GitHub

Commented plugin

```
(function($) {

$.fullslide = function(element, options) {

// Declare plugins variables
var autoInt; // variable to store the timing function

// Default options
var defaults = {
displayQty : 1,
moveQty : 1,
moveDuration : 1000,
moveEasing : "swing",
slideMargin : 20,
minWidth : "",
maxWidth : "",
autoSlide : true,
autoDirection : "left",
slideDelay : 3000,
pauseOnHover : true,
onReady : function() {},
onBeforeSlide : function() {},
onAfterSlide : function() {}
}

// Use "slider" to reference the current instance of the object
var slider = this;

// This will hold the merged default, and user-provided options
slider.settings = {}

// Set vars to be used from outside of plugin, specifically for event bindings
slider.vars = {}

var $element = $(element), // Reference to the jQuery version of DOM element
element = element; // Reference to the actual DOM element

slider.init = function() {
/**
* The "constructor" method - set the slider up at runtime,
* including creating DOM elements and setting widths.
*/

// Declare method's variables
var fullslideLi,
ulH,
loaded,
fontSize;

//

Solution

From a once over;

  • Very well commented



-
Assigning element to element is overkill here and does nothing:

var $element = $(element), // Reference to the jQuery version of DOM element
     element = element;    // Reference to the actual DOM element


-
From a naming perspective, I would refrain from using the tag type in the variable name:

// Declare method's variables
var fullslideLi,
    ulH,
    loaded,
    fontSize;


You are tying yourself to an implementation detail with the name, you could go for element or el in those variable names

-
Consider Math.min here:

// Ensure we cannot move more slides than we have
    if( slider.settings.moveQty > origSlideQty ) {
        slider.settings.moveQty = origSlideQty;
    }

    // Ensure we don't try to move more than we are displaying
    if( slider.settings.moveQty > slider.settings.displayQty ) {
        slider.settings.moveQty = slider.settings.displayQty;
    }


could be

slider.settings.moveQty = Math.min( origSlideQty, slider.settings.moveQty , slider.settings.displayQty );


furthermore from a DRY perspective I would probably have moved slider.settings to a local var settings:

settings.moveQty = Math.min( origSlideQty, settings.moveQty , settings.displayQty );


  • window.setTimeout returns a timeoutID, you should store this timeoutID and clear it after the timeout. And then only call window.setTimeout if the timeoutID is cleared out. I hope that makes sense. It should prevent your problems



-
You can set more than 1 property with .css:

// Apply the height to the UL
                $element.css({
                    height : ulH + "px"
                });

                // Show the slider if hidden
                $element.css({
                    opacity : 1
                });


could be

// Apply the height to the UL and show the slider
                $element.css({
                    height : ulH + "px", 
                    opacity : 1
                });

Code Snippets

var $element = $(element), // Reference to the jQuery version of DOM element
     element = element;    // Reference to the actual DOM element
// Declare method's variables
var fullslideLi,
    ulH,
    loaded,
    fontSize;
// Ensure we cannot move more slides than we have
    if( slider.settings.moveQty > origSlideQty ) {
        slider.settings.moveQty = origSlideQty;
    }

    // Ensure we don't try to move more than we are displaying
    if( slider.settings.moveQty > slider.settings.displayQty ) {
        slider.settings.moveQty = slider.settings.displayQty;
    }
slider.settings.moveQty = Math.min( origSlideQty, slider.settings.moveQty , slider.settings.displayQty );
settings.moveQty = Math.min( origSlideQty, settings.moveQty , settings.displayQty );

Context

StackExchange Code Review Q#29982, answer score: 5

Revisions (0)

No revisions yet.