jQuery plugin that makes a slider out of an <ul> list

Question

This is one of my very first jQuery plugins. In short: it makes a slider out of an <ul> list filled with <li> items containing <img>s. Just follow this JSFiddle link to see it in action.

I therefore wanted to know if there were any flaws in my code. I would like it be reviewed by anyone who would be willing to let me know of any major problem in it.

So here's the HTML:

<ul id="slider">
 <li><img src="http://lorempixel.com/700/300/food/1" alt="slider_1"/></li>
 <li><img src="http://lorempixel.com/700/300/food/2" alt="slider_2"/></li>
 <li><img src="http://lorempixel.com/700/300/food/3" alt="slider_3"/></li>
 <li><img src="http://lorempixel.com/700/300/food/4" alt="slider_4"/></li>
 <li><img src="http://lorempixel.com/700/300/food/5" alt="slider_4"/></li>
</ul>

And the jQuery plugin code:

;(function($) {
 $.fn.extend({
 sdSlider: function(options) {
 if (options && typeof(options) == 'object') {
 options = $.extend({}, $.sdSlider.defaults, options);
 }
 // be sure the plugin is attached to only one DOMElement
 if($(this).length == 1) {
 new $.sdSlider(this, options);
 } else if($(this).length > 1) {
 console.error($.messages.severalElements);
 } else if($(this).length == 0) {
 console.error($.messages.zeroElement);
 }
 return this;
 }
 });
 // error messages to be displayed in the console
 $.messages = {
 imgAndLiMismatch: '[jQuery.sdSlider] Error: the number of list items and the number of <img src> mismatch.',
 severalElements: '[jQuery.sdSlider] Error: several DOM element have been detected. Are you sure you\'re using the right selector?',
 noImgFound: '[jQuery.sdSlider] Error: no <img> tag was found within the <li>.',
 zeroElement: '[jQuery.sdSlider] Error: couldn\'t find the slider. It seems you have targetted no element.'
 };
 $.sdSlider = function(el, option) {
 $(window).on('load', function() {
 var options = option || $.sdSlider.defaults
 , $li = $(el).find('> li')
 , $img = $li.find('> img')
 , imgSrcs = []
 , imgHeights = []
 , imgWidths = []
 , $wrapper
 , i
 , index;
 if(!$img.length) {
 console.error($.messages.noImgFound);
 return;
 }
 // mark the first slide as active
 $(el).find('> li:first-child').addClass('active');
 // add border if asked to
 if(options.border && typeof(options.border) === 'object') {
 $img.css('border', options.border.width + 'px solid ' + options.border.color);
 }
 $img.each(function(i) {
 $(this).attr('data-image', i);
 imgSrcs.push($(this).attr('src'));
 imgHeights.push($(this).outerHeight());
 imgWidths.push($(this).outerWidth());
 });
 // if each li has an img
 if(imgSrcs.length === $li.length) {
 maxHeight = Math.max.apply(Math, imgHeights);
 maxWidth = Math.max.apply(Math, imgWidths);
 // build the wrapper div
 $wrapper = $('<div>').attr('id', 'sdSlider-wrapper').css({
 width: maxWidth,
 height: maxHeight
 });
 $(el).wrap($wrapper);
 // add arrows if asked to
 if(options.arrows) {
 $.sdSlider.addArrows(el, options, $wrapper);
 }
 // add dots controls if asked to
 if(options.controls) {
 $controlWrapper = $.sdSlider.addControls(el, options);
 // on click on the dots
 $controlWrapper.find('> span').on('click', function(e) {
 // make slider slide
 i = $(this).index();
 $.sdSlider.appear(el, options, i);
 });
 }
 // autostarting the slider if asked to 
 if(options.autoStart.active) {
 window.setInterval(function() {
 index = $(el).find('> li.active').index();
 if(index + 1 < $img.length) {
 $.sdSlider.appear(el, options, index + 1);
 } else {
 $.sdSlider.appear(el, options, index - $img.length + 1);
 }
 }, options.autoStart.delay);
 }
 } else {
 console.error($.messages.imgAndLiMismatch);
 }
 return;
 });
 };
 // function to add the dots control on the bottom of the slider
 $.sdSlider.addControls = function(el, options) {
 var $li = $(el).find('> li')
 , $controlWrapper = $('<div>')
 .addClass('control-wrapper')
 .css({
 'text-align': 'center',
 'bottom': '35px',
 'background-color': 'rgba(170, 170, 170, 0.5)'
 })
 , $controls;
 $li.each(function(i) {
 $controls = $('<span>').attr('data-image', i);
 if(!i) {
 $controls.addClass('active');
 }
 $controlWrapper.append($controls);
 });
 $(el).after($controlWrapper);
 return $controlWrapper;
 }
 // function to add the controlling left and right arrows on the sides
 $.sdSlider.addArrows = function(el, options, $wrapper) {
 var classes = 'sdArrow fa-4x fa'
 , $left = $('<i />').addClass(classes + ' sdArrow-left fa-arrow-circle-left disabled').css('left', 0)
 , $right = $('<i />').addClass(classes + ' sdArrow-right fa-arrow-circle-right').css('right', 0)
 , $img = $(el).find('> li').find('> img')
 , index;
 ;
 $(el).after($left).before($right);
 // if right arrow is clicked
 $right.on('click', function() {
 index = $(el).find('> li.active').index();
 if(index + 1 < $img.length) { // if this is not the end of the slider
 // make slider go right
 $.sdSlider.appear(el, options, index + 1);
 }
 });
 // if left arrow is clicked
 $left.on('click', function() {
 index = $(el).find('> li.active').index();
 if(index - 1 >= 0) { // if this is not the beginning of the slider
 // make slider go left
 $.sdSlider.appear(el, options, index - 1);
 }
 });
 return;
 };
 // function to make the slider slide (where 'i' is the index to go to)
 $.sdSlider.appear = function(el, options, i) {
 var activeImgIndex = $(el).find('> li.active').index()
 , animation = {}
 , gap = 0
 , $li = $(el).find('> li')
 , $img = $li.find('> img')
 , $left = $(el).parent('div').find('i.sdArrow-left')
 , $right = $(el).parent('div').find('i.sdArrow-right');
 // if the slider is not currently sliding
 if(!$li.is(':animated')) {
 $li.removeClass('active').eq(i).addClass('active');
 if(activeImgIndex < i) { // going right
 gap = i - activeImgIndex;
 animation['left'] = '-=' + ($li.find('> img').eq(i).outerWidth() * gap) + 'px';
 } else { // going left
 gap = activeImgIndex - i;
 animation['left'] = '+=' + ($li.find('> img').eq(i).outerWidth() * gap) + 'px';
 }
 // slider animation
 $li.each(function() {
 $(this).animate(animation, {
 duration: options.duration
 });
 });
 // add disabled class to arrows if needed to
 if(options.arrows) {
 if(i + 1 === $img.length) {
 $right.addClass('disabled');
 } else {
 $right.removeClass('disabled');
 }
 if(i === 0) {
 $left.addClass('disabled');
 } else {
 $left.removeClass('disabled');
 }
 }
 // add active class to corresponding dot
 $('.control-wrapper')
 .find('> span')
 .removeClass('active')
 .eq(i)
 .addClass('active')
 ;
 }
 return;
 };
 // plugin default options
 $.sdSlider.defaults = {
 autoStart: {
 active: false,
 delay: 1000
 },
 border: {
 color: '#000',
 width: 0
 },
 controls: true,
 arrows: true,
 duration: 1000
 };
})(jQuery);

The code to initiate the plugin is as such:

jQuery(function($) {
 $('#slider').sdSlider({
 autoStart: { // an object containing:
 active: false, // a boolean indicating whether to start the slider automatically
 delay: 1000 // and a integer specifying the delay between each slide
 },
 border: { // an object containing:
 color: '#f00', // the color of each image border as a string 
 width: 0 // the width in px of each image border as an integer
 },
 controls: true, // a boolean to specify whether to display the dots controlling each slide
 arrows: true, // a boolean to specify whether to display the left and right arrows
 duration: 500 // the duration in ms of each animation as a integer
 });
});

You can see a more complete README on my Github account.

I would also like to know if my code is not cross-browser. Please feel free to spare no details about this point and how it can be fixed.

My general concerns are about following the best practices in order to write a good jQuery plugin so I'm looking for any suggestions on how to improve this code.

Answer 1

One thing that jumps out to me is locking your plugin to one element:

$.fn.extend({
 sdSlider: function(options) {
 if (options && typeof(options) == 'object') {
 options = $.extend({}, $.sdSlider.defaults, options);
 }
 // be sure the plugin is attached to only one DOMElement
 if($(this).length == 1) {
 new $.sdSlider(this, options);
 } else if($(this).length > 1) {
 console.error($.messages.severalElements);
 } else if($(this).length == 0) {
 console.error($.messages.zeroElement);
 }
 return this;
 }
});

Most anything jQuery will happily accept multiple items so I propose the following change:

$.fn.extend({
 sdSlider: function(options) {
 if (options && typeof(options) == 'object') {
 options = $.extend({}, $.sdSlider.defaults, options);
 }
 if($(this).length == 0) {
 console.error($.messages.zeroElement);
 }
 return this.each(function(index, el){
 new $.sdSlider(el, options);
 });
 }
});

along with replacing:

$wrapper = $('<div>').attr('id', 'sdSlider-wrapper').css({...})

with:

$wrapper = $('<div>').addClass('sdSlider-wrapper').css({...})

As well as updating any relevant css.

jsFiddle

Answer 2

I saw something that could make your code more dry...

 // be sure the plugin is attached to only one DOMElement
 if($(this).length == 1) {
 new $.sdSlider(this, options);
 } else if($(this).length > 1) {
 console.error($.messages.severalElements);
 } else if($(this).length == 0) {
 console.error($.messages.zeroElement);
 }

you should grab $(this).length and make a variable out of it. it would be so much cleaner like this

var objectLength = $(this).length;
if(objectLength == 1) {
 new $.sdSlider(this, options);
} else if(objectLength > 1) {
 console.error($.messages.severalElements);
} else if(objectLength == 0) {
 console.error($.messages.zeroElement);
}

but you can probably come up with a better name for the variable.

---

Here is a place that you should probably do the opposite.

if(options.controls) {
 $controlWrapper = $.sdSlider.addControls(el, options);
 // on click on the dots
 $controlWrapper.find('> span').on('click', function(e) {
 // make slider slide
 i = $(this).index();
 $.sdSlider.appear(el, options, i);
 });
}

take i out and

if(options.controls) {
 $controlWrapper = $.sdSlider.addControls(el, options);
 // on click on the dots
 $controlWrapper.find('> span').on('click', function(e) {
 // make slider slide
 $.sdSlider.appear(el, options, $(this).index());
 });
}

---

You have a couple of places in the code where you have several new lines one right after another.

Don't do that, you could end up putting code in there and breaking stuff.

Take a look here...

 // if each li has an img
 if(imgSrcs.length === $li.length) {
 maxHeight = Math.max.apply(Math, imgHeights);
 maxWidth = Math.max.apply(Math, imgWidths);
 // build the wrapper div
 $wrapper = $('<div>').attr('id', 'sdSlider-wrapper').css({
 width: maxWidth,
 height: maxHeight
 });
 $(el).wrap($wrapper);
 // add arrows if asked to
 if(options.arrows) {
 $.sdSlider.addArrows(el, options, $wrapper);
 }
 // add dots controls if asked to
 if(options.controls) {
 $controlWrapper = $.sdSlider.addControls(el, options);
 // on click on the dots
 $controlWrapper.find('> span').on('click', function(e) {
 // make slider slide
 i = $(this).index();
 $.sdSlider.appear(el, options, i);
 });
 }
 // autostarting the slider if asked to 
 if(options.autoStart.active) {
 window.setInterval(function() {
 index = $(el).find('> li.active').index();
 if(index + 1 < $img.length) {
 $.sdSlider.appear(el, options, index + 1);
 } else {
 $.sdSlider.appear(el, options, index - $img.length + 1);
 }
 }, options.autoStart.delay);
 }
 } else {
 console.error($.messages.imgAndLiMismatch);
 }

Right before that else statement. If someone puts something in there that they think is going to be after the if statement, or even puts another else statement on that not seeing the already existing else statement.

I know it's unlikely, but it happens, be wary of too much white space in your code.

---

You shouldn't add CSS to an object with JavaScript, it isn't maintainable.

 , $controlWrapper = $('<div>')
 .addClass('control-wrapper')
 .css({
 'text-align': 'center',
 'bottom': '35px',
 'background-color': 'rgba(170, 170, 170, 0.5)'
 })

You already add a class to this div anyway, why not use a style sheet to add the style, that way you don't have to look through the code to change the background-color or something like that.

---

I wanted to say something about the comments, but I couldn't find anything bad to say about them. At first I thought maybe they were going to be extraneous, but they weren't, they were informative and straight to the point.

They seemed like good comments to me.