patternjavascriptMinor
jQuery carousel plugin
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
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;
//
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;
-
Assigning
-
From a naming perspective, I would refrain from using the tag type in the variable name:
You are tying yourself to an implementation detail with the name, you could go for
-
Consider
could be
furthermore from a DRY perspective I would probably have moved
-
You can set more than 1 property with
could be
- 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.setTimeoutreturns a timeoutID, you should store this timeoutID and clear it after the timeout. And then only callwindow.setTimeoutif 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.