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

jQuery plugin - image slider

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

Problem

This is my first jQuery plugin (used the jQuery ui widget factory). All code improvements are welcome.

GitHub

The plugin creates a gallery with one big picture and a given amount of clickable thumbnails. When the last thumbnail of set has been clicked it fades into the next thumbnail set.

```
;(function ($, window, document, undefined) {

$.widget("company.imageSlider", {

options: {
debug: false,
templateId: "",
thumbAmount: 4,
photos: [],
imagePath: "./img/landscape/",
thumbPath: "./img/thumbs/"
},

thumbSetPosition: {
val: 0
},

thumbSetZindex: {
val: 0
},

_create: function () {
this.options.debug == true && console.log("Create has been called");

this.thumbSetPosition = 1;
this.thumbSetZindex = 999;
this.element.addClass("plugin-is-active");
},

_getPhotosByRange: function (from, to) {
return this.options.photos.slice(from, to)
},

_getPhotoCount: function() {
return this.options.photos.length;
},

_buildMainFrame: function () {
this.options.debug == true && console.log("Building main image frame");

var widget = this;
var container = $(".company-image-slider__main-image");

// Add mouse over events
container.on("mouseenter", function () {
$(this).find(".company-image-slider__nav").fadeIn(500);
}).on("mouseleave", function () {
$(this).find(".company-image-slider__nav").fadeOut(500);
});

// Add on click event
container.find(".company-image-slider__nav").on("click", function () {
if ($(this).data("direction") == "next") {
widget._nextImage();
} else {
widget._prevImage();
}
});
},

_nextImage: function () {
this.options.debug == true && console.log("Selecting next image");

var activeImage = $(".company-i

Solution

Overall, I would say the code is well written. I can't speak about the jQueryUI parts because I am not as familiar with it. However, there are a couple of things you can do to improve it though. Most of these would be what I would call micro-optimizations.

First, you need to DRY your code as much as possible. For instance, you repeat this line a lot.

this.options.debug == true && console.log("Selecting previous image");


If you find you are repeating a line of code then it really should be a function. Also, there is no need to check for a value of true.

_log : function ( txt ) {
  this.options.debug && console.log( txt );
}


Another line that is used repeatedly is the one similar to below:

prevImage.trigger("click", {widget: this, direction: "prev"}, this._scrollToThumbSet);


Again, you can make a function out of this:

_triggerClick :  function ( el, dir, fn ) {
  el.trigger("click", {widget: this, direction: dir }, fn);
}

this._triggerClick( prevImage, "prev", this._scrollToThumbSet );
this._triggerClick( activeImage, "prev, this._scrollToThumbSet );


The other thing is you have some very long functions. These should probably be broken up into smaller sub functions. For instance, your _buldThumbs is huge. just the $.each is over 20 lines long.

Also, you have (i == 0) checks three separate times in that function. You should use === instead. But there should be a way to simplify that code.

var isFirst = !!(!i); // !i is same as i===0 !! forces a boolean value

var thumb = ( isFirst ) ? template.clone() : template.find(".company-image-slider__thumb-container__thumb-set").clone();


You should also cache all of your selections that occur more once. You do this in most cases, but there are a few places you didn't. In the _handleThumbClick function, you have $(this) twice. Go ahead and set a variable:

var $t = $(this);
$t.siblings().removeClass("is-active");
$t.addClass("is-active");


Or just chain it together:

$(this).addClass("is-active").siblings().removeClass("is-active");


If all of your slideshow is inside of a single div container, you can do a selection on that container, cache it, and use it and .find to do all of your other selections. This would improve jQuery's performance.

var $ss = $('.company-image-slider');  //or whatever
var thumbContainer = $ss.find('.company-image-slider__thumb-container');
//etc etc


Lastly, in your destroy function you check to see if the element has the class before you remove it. You can call removeClass either way. If the class is there, it is removed; otherwise it does nothing.

I hope that helps.
Feel free to leave a comment if you have questions.

Code Snippets

this.options.debug == true && console.log("Selecting previous image");
_log : function ( txt ) {
  this.options.debug && console.log( txt );
}
prevImage.trigger("click", {widget: this, direction: "prev"}, this._scrollToThumbSet);
_triggerClick :  function ( el, dir, fn ) {
  el.trigger("click", {widget: this, direction: dir }, fn);
}

this._triggerClick( prevImage, "prev", this._scrollToThumbSet );
this._triggerClick( activeImage, "prev, this._scrollToThumbSet );
var isFirst = !!(!i); // !i is same as i===0 !! forces a boolean value

var thumb = ( isFirst ) ? template.clone() : template.find(".company-image-slider__thumb-container__thumb-set").clone();

Context

StackExchange Code Review Q#92557, answer score: 3

Revisions (0)

No revisions yet.