I have made this custom image carousel, using HTML5, jQuery and CSS. My aim was to make it really lightweight but with (just) enough features: "bullets", auto-advance, responsiveness.

var $elm = $('.slider'),
  $slidesContainer = $elm.find('.slides-container'),
  slides = $slidesContainer.children('a'),
  slidesCount = slides.length,
  slideHeight = $(slides[0]).find('img').outerHeight(false),
  animationspeed = 1500,
  animationInterval = 7000;

var shuffle = function(slides) {
  var j, x, i;
  for (i = slides.length - 1; i > 0; i--) {
    j = Math.floor(Math.random() * (i + 1));
    x = slides[i];
    slides[i] = slides[j];
    slides[j] = x;
  }
  return slides;
}

shuffle(slides);

// Set (initial) z-index for each slide
var setZindex = function() {
  for (var i = 0; i < slidesCount; i++) {
    $(slides[i]).css('z-index', slidesCount - i);
  }
};
setZindex();

var setActiveSlide = function() {
  $(slides).removeClass('active');
  $(slides[activeIdx]).addClass('active');
};

var advanceFunc = function() {
  if ($('.slider-nav li.activeSlide').index() + 1 != $('.slider-nav li').length) {
    $('.slider-nav li.activeSlide').next().find('a').trigger('click');
  } else {
    $('.slider-nav li:first').find('a').trigger('click');
  }
}

var autoAdvance = setInterval(advanceFunc, animationInterval);

//Set slide height
$(slides).css('height', slideHeight);

// Append bullets
if (slidesCount > 1) {
  /* Prepend the slider navigation to the slider
     if there are at least 2 slides */
  $elm.prepend('<ul class="slider-nav"></ul>');

  // make a bullet for each slide
  for (var i = 0; i < slidesCount; i++) {
    var bullets = '<li><a href="#">' + i + '</a></li>';
    if (i == 0) {
      // active bullet
      var bullets = '<li class="activeSlide"><a href="#">' + i + '</a></li>';
      // active slide
      $(slides[0]).addClass('active');
    }
    $('.slider-nav').append(bullets);
  }
};

var Queue = function() {
  var lastPromise = null;

  this.add = function(callable) {
    var methodDeferred = $.Deferred();
    var queueDeferred = this.setup();
    // execute next queue method
    queueDeferred.done(function() {

      // call actual method and wrap output in deferred
      callable().then(methodDeferred.resolve)
    });
    lastPromise = methodDeferred.promise();
  };

  this.setup = function() {
    var queueDeferred = $.Deferred();
    // when the previous method returns, resolve this one
    $.when(lastPromise).always(function() {
      queueDeferred.resolve();
    });
    return queueDeferred.promise();
  }
};

var queue = new Queue();
var slideUpDown = function(previousIdx, activeIdx) {
  queue.add(function() {
    return new Promise(function(resolve, reject) {
      // set top property for all the slides
      $(slides).not(slides[previousIdx]).css('top', slideHeight);
      // then animate to the next slide
      $(slides[activeIdx]).animate({
        'top': 0
      }, animationspeed);

      $(slides[previousIdx]).animate({
        'top': "-100%"
      }, animationspeed, 'swing', resolve);
    })
  })
};

var previousIdx = '0' // First slide
$('.slider-nav a').on('click', function(event) {
  event.preventDefault();
  activeIdx = $(this).text();

  // Disable clicling on an active item
  if ($(slides[activeIdx]).hasClass("active")) {
    return false;
  }
  $('.slider-nav a').closest('li').removeClass('activeSlide');
  $(this).closest('li').addClass('activeSlide');

  // Reset autoadvance if user clicks bullet
  if (event.originalEvent !== undefined) {
    clearInterval(autoAdvance);
    autoAdvance = setInterval(advanceFunc, animationInterval);
  }

  setActiveSlide();
  slideUpDown(previousIdx, activeIdx);
  previousIdx = activeIdx
});
body * {
  box-sizing: border-box;
}

.container {
  max-width: 1200px;
  margin: 0 auto;
}

.slider {
  width: 100%;
  height: 300px;
  position: relative;
  overflow: hidden;
}

.slider .slider-nav {
  text-align: center;
  position: absolute;
  padding: 0;
  margin: 0;
  left: 10px;
  right: 10px;
  bottom: 2px;
  z-index: 10;
}

.slider .slider-nav li {
  display: inline-block;
  width: 20px;
  height: 4px;
  margin: 0 1px;
  text-indent: -9999px;
  overflow: hidden;
  background-color: rgba(255, 255, 255, .5);
}

.slider .slider-nav a {
  display: block;
  height: 4px;
  line-height: 4px;
}

.slider .slider-nav li.activeSlide {
  background: #fff;
}

.slider .slider-nav li.activeSlide a {
  display: none;
}

.slider .slider-container {
  width: 100%;
  text-align: center;
}

.slider .slides-container a {
  display: block;
  position: absolute;
  top: 0;
  left: 0;
  right: 0;
}

.slider .slides-container img {
  transform: translateX(-50%);
  margin-left: 50%;
}
<script src="https://ajax.googleapis.com/ajax/libs/jquery/3.1.1/jquery.min.js"></script>

<div class="container">
  <div class="slider">
    <div class="slides-container">
      <a href="#">
        <img src="https://picsum.photos/1200/300/?gravity=east" alt="">
      </a>
      <a href="#">
        <img src="https://picsum.photos/1200/300/?gravity=south" alt="">
      </a>
      <a href="#">
        <img src="https://picsum.photos/1200/300/?gravity=west" alt="">
      </a>
      <a href="#">
        <img src="https://picsum.photos/1200/300/?gravity=north" alt="">
      </a>
    </div>
  </div>
</div>

It works fine but I am certain it can be optimized: the same result can be achieved with less code. I am not thinking of a complete rewrite, but of ditching redundant and useless code.

The carousel's current architecture requires the slides to have z-indexes which I wish I could get rid of. What would be a good alternative to that?

See a jsFiddle HERE.

Please help me make it better. Thanks!

  • 1
    Is the queue with promises supposed to prevent the slides from moving too quickly or some other purpose? – Sᴀᴍ Onᴇᴌᴀ Sep 5 at 20:48
  • @SᴀᴍOnᴇᴌᴀ Yes, this is the purpose. – Razvan Zamfir Sep 14 at 13:26
  • Why do you believe z-indexes need to be set? The snippet in the first version (without those set) appears to function the same as the snippet in the current version... – Sᴀᴍ Onᴇᴌᴀ Sep 14 at 16:18
up vote 0 down vote accepted
+50

General Feedback

The carousel appears to work acceptably. The code is a bit scattered. Consider the variable activeIdx. It appears to be set in the click handler and referenced in the function setActiveSlide() as a global variable - not as a parameter, but in slideUpDown() it is a parameter.

I'm not convinced the promise queue is absolutely necessary. Perhaps a simple debounced function would suffice.

And there are a lot of repeated DOM queries - remember those are not cheap! Especially in the function advanceFunc(). Instead of querying the DOM so many times, it would be better to store the list items (A.K.A. bullets) in a variable after they are added and re-use them in advanceFunc(). Modulus division can then be used to determine the next index.

Specific critique

bullet creation

The name of the variable bullets is a bit misleading for a single element. A singular name like bullet would be more appropriate.

And to create each bullet, the jQuery function could be used. So instead of manually constructing the HTML for the list items:

var bullets = '<li><a href="#">' + i + '</a></li>';
if (i == 0) {
  // active bullet
  var bullets = '<li class="activeSlide"><a href="#">' + i + '</a></li>';

use $('<li>') for the bullet and .html() to set the inner HTML:

var bullet = $('<li>').html('<a href="#">' + i + '</a>');

Then the class name can be added via .addClass()

if (i == 0) {
  $(slides[0]).addClass('active');
  bullet.addClass('activeSlide');

That way the inner HTML is only specified once.

Shuffle function return value unused

There is no need to return slides at the end of shuffle():

return slides;

This is because the return value is not assigned to anything (unless you intended for that to be the case):

shuffle(slides);

Rewrite

See the modified code below. It doesn't use the queue or promises at all, and as far as I can tell maintains the same functionality. I also made previousIdx and activeIdx variables outside the functions instead of parameters. Because of this, I wrapped the whole thing in an IIFE to avoid adding those variables to the global scope.

$(function() {
  var $elm = $('.slider'),
    $slidesContainer = $elm.find('.slides-container'),
    slides = $slidesContainer.children('a'),
    slidesCount = slides.length,
    slideHeight = $(slides[0]).find('img').outerHeight(false),
    animationspeed = 1500,
    animationInterval = 7000;

  var activeIdx = 0;
  var previousIdx = 0; // First slide
  var shuffle = function(slides) {
    var j, x, i;
    for (i = slides.length - 1; i > 0; i--) {
      j = Math.floor(Math.random() * (i + 1));
      x = slides[i];
      slides[i] = slides[j];
      slides[j] = x;
    }
    return slides;
  }
  shuffle(slides);

  // Set (initial) z-index for each slide
  var setZindex = function() {
    for (var i = 0; i < slidesCount; i++) {
      $(slides[i]).css('z-index', slidesCount - i);
    }
  };
  setZindex();

  var setActiveSlide = function() {
    $(slides).removeClass('active');
    $(slides[activeIdx]).addClass('active');
    $(bullets).removeClass('activeSlide');
    $(bullets[activeIdx]).addClass('activeSlide');
  };

  function showSlideAtActiveIndex(resetInterval) {
    setActiveSlide();
    slideUpDown(); //previousIdx, activeIdx);
    previousIdx = activeIdx;
  }

  var advanceFunc = function() {
    activeIdx = ++activeIdx % slidesCount;
    showSlideAtActiveIndex();
  }

  var autoAdvance = setInterval(advanceFunc, animationInterval);

  //Set slide height
  $(slides).css('height', slideHeight);

  // Append bullets
  if (slidesCount > 1) {
    /* Prepend the slider navigation to the slider
       if there are at least 2 slides */
    $elm.prepend('<ul class="slider-nav"></ul>');

    // make a bullet for each slide
    for (var i = 0; i < slidesCount; i++) {
      var bullet = $('<li>').html('<a href="#">' + i + '</a>');
      if (i == 0) {
        bullet.addClass('activeSlide');
        // active bullet
        // active slide
        $(slides[0]).addClass('active');
      }
      $('.slider-nav').append(bullet);
    }
  };
  var bullets = $('.slider-nav li');

  var slideUpDown = function() {
    $(slides).not(slides[previousIdx]).css('top', slideHeight);
    // then animate to the next slide
    $(slides[activeIdx]).animate({
      'top': 0
    }, animationspeed);
    $(slides[previousIdx]).animate({
      'top': "-100%"
    }, animationspeed, 'swing');
  };

  $('.slider-nav a').on('click', function(event) {
    var clickedIdx = $(this).text();
    if ($(slides[clickedIdx]).hasClass("active")) {
      return false;
    }
    activeIdx = clickedIdx;
    showSlideAtActiveIndex(); 
    clearInterval(autoAdvance);
    setTimeout(function() {
      autoAdvance = setInterval(advanceFunc, animationInterval);
    }, animationInterval);
  });
});
body * {
  box-sizing: border-box;
}

.container {
  max-width: 1200px;
  margin: 0 auto;
}

.slider {
  width: 100%;
  height: 300px;
  position: relative;
  overflow: hidden;
}

.slider .slider-nav {
  text-align: center;
  position: absolute;
  padding: 0;
  margin: 0;
  left: 10px;
  right: 10px;
  bottom: 2px;
  z-index: 10;
}

.slider .slider-nav li {
  display: inline-block;
  width: 20px;
  height: 4px;
  margin: 0 1px;
  text-indent: -9999px;
  overflow: hidden;
  background-color: rgba(255, 255, 255, .5);
}

.slider .slider-nav a {
  display: block;
  height: 4px;
  line-height: 4px;
}

.slider .slider-nav li.activeSlide {
  background: #fff;
}

.slider .slider-nav li.activeSlide a {
  display: none;
}

.slider .slider-container {
  width: 100%;
  text-align: center;
}

.slider .slides-container a {
  display: block;
  position: absolute;
  top: 0;
  left: 0;
  right: 0;
}

.slider .slides-container img {
  transform: translateX(-50%);
  margin-left: 50%;
}
<script src="https://ajax.googleapis.com/ajax/libs/jquery/3.1.1/jquery.min.js"></script>

<div class="container">
  <div class="slider">
    <div class="slides-container">
      <a href="#">
        <img src="https://picsum.photos/1200/300/?gravity=east" alt="">
      </a>
      <a href="#">
        <img src="https://picsum.photos/1200/300/?gravity=south" alt="">
      </a>
      <a href="#">
        <img src="https://picsum.photos/1200/300/?gravity=west" alt="">
      </a>
      <a href="#">
        <img src="https://picsum.photos/1200/300/?gravity=north" alt="">
      </a>
    </div>
  </div>
</div>

  • Feel free to improve my version. This is the reason it is posted here. Thank you! – Razvan Zamfir Sep 17 at 18:45
  • In fact, click the second bullet of the carousel on your site and you will see that the second slide replaces the first instantly, with no transition. – Razvan Zamfir Sep 17 at 19:45
  • The shuffle(slides); line "randomizes" the slides. – Razvan Zamfir Sep 18 at 9:58
  • Okay I added a modified snippet. I understand the point of shuffle() but am stating that there is no need for return slides; at the end – Sᴀᴍ Onᴇᴌᴀ Sep 18 at 18:42
  • It seems to look and behave differently: the slides do not advance one after the other (does this happen in the snippet window only?). Also, the z-indexes are still there. – Razvan Zamfir Sep 19 at 8:45

Your Answer

 

By clicking "Post Your Answer", you acknowledge that you have read our updated terms of service, privacy policy and cookie policy, and that your continued use of the website is subject to these policies.

Not the answer you're looking for? Browse other questions tagged or ask your own question.