patternjavascriptMinor
jQuery plugin - image slider
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
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.
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
Another line that is used repeatedly is the one similar to below:
Again, you can make a function out of this:
The other thing is you have some very long functions. These should probably be broken up into smaller sub functions. For instance, your
Also, you have
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
Or just chain it together:
If all of your slideshow is inside of a single
Lastly, in your
I hope that helps.
Feel free to leave a comment if you have questions.
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 etcLastly, 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.