Thumbnails for bootstrap carousel jQuery plugin

Question

Codepen: http://codepen.io/srkimir/pen/mGbrf
Github: https://github.com/srkimir/thumbnails-carousel

While you slide left or right appropriately, thumbnails gets selected and change their opacity to be different among others thumbnails. You can also click on thumbnails to show the appropriate image on carousel.

enter image description here
HTML

Add child <ul> of thumbnails in the end of carousel bootstrap parent, like:

<div id="carousel-example-generic" class="carousel slide" data-ride="carousel">
 <!-- Bootstrap carousel code -->
 ...
 <!-- Thumbnails -->
 <ul class="thumbnails-carousel clearfix">
 <li><img src="images/1_tn.jpg" alt="1_tn.jpg"></li>
 <li><img src="images/2_tn.jpg" alt="1_tn.jpg"></li>
 <li><img src="images/3_tn.jpg" alt="1_tn.jpg"></li>
 <li><img src="images/4_tn.jpg" alt="1_tn.jpg"></li>
 </ul>
</div>

CSS

ul.thumbnails-carousel {
 padding: 5px 0 0 0;
 margin: 0;
 list-style-type: none;
 text-align: center;
}
ul.thumbnails-carousel .center {
 display: inline-block;
}
ul.thumbnails-carousel li {
 margin-right: 5px;
 float: left;
 cursor: pointer;
}
.controls-background-reset {
 background: none !important;
}
.active-thumbnail {
 opacity: 0.4;
}
.indicators-fix {
 bottom: 70px;
}

JavaScript:

// thumbnails.carousel.js jQuery plugin
;(function(window, ,ドル undefined) {
 var conf = {
 center: true,
 backgroundControl: false
 };
 var cache = {
 $carouselContainer: $('.thumbnails-carousel').parent(),
 $thumbnailsLi: $('.thumbnails-carousel li'),
 $controls: $('.thumbnails-carousel').parent().find('.carousel-control')
 };
 function init() {
 cache.$carouselContainer.find('ol.carousel-indicators').addClass('indicators-fix');
 cache.$thumbnailsLi.first().addClass('active-thumbnail');
 if(!conf.backgroundControl) {
 cache.$carouselContainer.find('.carousel-control').addClass('controls-background-reset');
 }
 else {
 cache.$controls.height(cache.$carouselContainer.find('.carousel-inner').height());
 }
 if(conf.center) {
 cache.$thumbnailsLi.wrapAll("<div class='center clearfix'></div>");
 }
 }
 function refreshOpacities(domEl) {
 cache.$thumbnailsLi.removeClass('active-thumbnail');
 cache.$thumbnailsLi.eq($(domEl).index()).addClass('active-thumbnail');
 } 
 function bindUiActions() {
 cache.$carouselContainer.on('slide.bs.carousel', function(e) {
 refreshOpacities(e.relatedTarget);
 });
 cache.$thumbnailsLi.click(function(){
 cache.$carouselContainer.carousel($(this).index());
 });
 }
 $.fn.thumbnailsCarousel = function(options) {
 conf = $.extend(conf, options);
 init();
 bindUiActions();
 return this;
 }
})(window, jQuery);
// Kick it
$('.thumbnails-carousel').thumbnailsCarousel({});

Additionally, you can pass center (default: true) and backgroundControl (default: false) if you want (or no) to center your thumbnails or backgroundControl if you want (or no) controls background which is activated on hover in bootstrap carousel.

Answer 1

• The only thing worse than missing alt text is bad alt text. 1_tn.jpg is meaningless. Instead, use empty alt text.

• Your CSS looks okay. I'll note you can use padding: 5px 0 0;, with padding-left being automatically filled in.

• You never use window in your plugin, so you don't need to pass it.

• conf is shared across all instances of the plugin. Normally this is fine... but you're mutating conf, instead of using a separate variable to hold the defaults. This means that code like the below won't work as expected:

  $(".thumbnails-carousel.not-centered").thumbnailsCarousel({ center: false });
  // no need to specify because it's centered by default
  $(".thumbnails-carousel.centered").thumbnailsCarousel();
  

• Similarly, you are using a global shared "cache" which apparently holds a jQuery object created from a hard-coded string. In doing so, I am no longer able to:

  • dynamically generate the carousel
  • give the carousel my own class name
  • use more than one carousel in a non-buggy way

  Instead, use instance variables. After all, you're providing $.fn.thumbnailsCarousel. When you call $(".thumbnails-carousel").thumbnailsCarousel(), $(".thumbnails-carousel") is passed in as this!

• refreshOpacities() is oddly named. After going through it twice I now understand that it's just synchronising the active thumbnail with the active carousel image. Can you rename this function?

• bindUiActions() is okay but the lowercase i is bugging me... although some may argue that this is perfectly fine (à la getElementById()...).

• There's no point in explicitly specifying the empty object when calling .thumbnailsCarousel(), since $.extend(x, undefined) === x.

The following point becomes less relevant as you address the above points, mostly because I believe you should refactor the referenced code out anyway.

• In init(), you use cache.$carouselContainer.find(".carousel-control"), which is plain silly because you already have cache.$controls.

• Schism: NB: There are many other things to improve, but I think fixing point 5 is critically important. Other improvements can come in another iteration, or another answerer may choose to address them.