jQuery image carousel












3














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 {
height: 300px;
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!










share|improve this question




















  • 1




    Is the queue with promises supposed to prevent the slides from moving too quickly or some other purpose?
    – Sᴀᴍ Onᴇᴌᴀ
    Sep 5 '18 at 20:48










  • @SᴀᴍOnᴇᴌᴀ Yes, this is the purpose.
    – Razvan Zamfir
    Sep 14 '18 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 '18 at 16:18










  • After getting an answer you are not allowed to change your code anymore. This is to ensure that answers do not get invalidated and have to hit a moving target. If you have changed your code you can either post it as an answer (if it would constitute a code review) or ask a new question with your changed code (linking back to this one as reference). Refer to this post for more information
    – Sᴀᴍ Onᴇᴌᴀ
    16 hours ago
















3














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 {
height: 300px;
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!










share|improve this question




















  • 1




    Is the queue with promises supposed to prevent the slides from moving too quickly or some other purpose?
    – Sᴀᴍ Onᴇᴌᴀ
    Sep 5 '18 at 20:48










  • @SᴀᴍOnᴇᴌᴀ Yes, this is the purpose.
    – Razvan Zamfir
    Sep 14 '18 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 '18 at 16:18










  • After getting an answer you are not allowed to change your code anymore. This is to ensure that answers do not get invalidated and have to hit a moving target. If you have changed your code you can either post it as an answer (if it would constitute a code review) or ask a new question with your changed code (linking back to this one as reference). Refer to this post for more information
    – Sᴀᴍ Onᴇᴌᴀ
    16 hours ago














3












3








3







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 {
height: 300px;
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!










share|improve this question















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 {
height: 300px;
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!






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 {
height: 300px;
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>





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 {
height: 300px;
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>






javascript jquery css image html5






share|improve this question















share|improve this question













share|improve this question




share|improve this question








edited 17 hours ago







Razvan Zamfir

















asked Sep 4 '18 at 9:04









Razvan ZamfirRazvan Zamfir

9211




9211








  • 1




    Is the queue with promises supposed to prevent the slides from moving too quickly or some other purpose?
    – Sᴀᴍ Onᴇᴌᴀ
    Sep 5 '18 at 20:48










  • @SᴀᴍOnᴇᴌᴀ Yes, this is the purpose.
    – Razvan Zamfir
    Sep 14 '18 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 '18 at 16:18










  • After getting an answer you are not allowed to change your code anymore. This is to ensure that answers do not get invalidated and have to hit a moving target. If you have changed your code you can either post it as an answer (if it would constitute a code review) or ask a new question with your changed code (linking back to this one as reference). Refer to this post for more information
    – Sᴀᴍ Onᴇᴌᴀ
    16 hours ago














  • 1




    Is the queue with promises supposed to prevent the slides from moving too quickly or some other purpose?
    – Sᴀᴍ Onᴇᴌᴀ
    Sep 5 '18 at 20:48










  • @SᴀᴍOnᴇᴌᴀ Yes, this is the purpose.
    – Razvan Zamfir
    Sep 14 '18 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 '18 at 16:18










  • After getting an answer you are not allowed to change your code anymore. This is to ensure that answers do not get invalidated and have to hit a moving target. If you have changed your code you can either post it as an answer (if it would constitute a code review) or ask a new question with your changed code (linking back to this one as reference). Refer to this post for more information
    – Sᴀᴍ Onᴇᴌᴀ
    16 hours ago








1




1




Is the queue with promises supposed to prevent the slides from moving too quickly or some other purpose?
– Sᴀᴍ Onᴇᴌᴀ
Sep 5 '18 at 20:48




Is the queue with promises supposed to prevent the slides from moving too quickly or some other purpose?
– Sᴀᴍ Onᴇᴌᴀ
Sep 5 '18 at 20:48












@SᴀᴍOnᴇᴌᴀ Yes, this is the purpose.
– Razvan Zamfir
Sep 14 '18 at 13:26




@SᴀᴍOnᴇᴌᴀ Yes, this is the purpose.
– Razvan Zamfir
Sep 14 '18 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 '18 at 16:18




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 '18 at 16:18












After getting an answer you are not allowed to change your code anymore. This is to ensure that answers do not get invalidated and have to hit a moving target. If you have changed your code you can either post it as an answer (if it would constitute a code review) or ask a new question with your changed code (linking back to this one as reference). Refer to this post for more information
– Sᴀᴍ Onᴇᴌᴀ
16 hours ago




After getting an answer you are not allowed to change your code anymore. This is to ensure that answers do not get invalidated and have to hit a moving target. If you have changed your code you can either post it as an answer (if it would constitute a code review) or ask a new question with your changed code (linking back to this one as reference). Refer to this post for more information
– Sᴀᴍ Onᴇᴌᴀ
16 hours ago










1 Answer
1






active

oldest

votes


















1





+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>








share|improve this answer























  • Feel free to improve my version. This is the reason it is posted here. Thank you!
    – Razvan Zamfir
    Sep 17 '18 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 '18 at 19:45










  • The shuffle(slides); line "randomizes" the slides.
    – Razvan Zamfir
    Sep 18 '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 '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 '18 at 8:45











Your Answer





StackExchange.ifUsing("editor", function () {
return StackExchange.using("mathjaxEditing", function () {
StackExchange.MarkdownEditor.creationCallbacks.add(function (editor, postfix) {
StackExchange.mathjaxEditing.prepareWmdForMathJax(editor, postfix, [["\$", "\$"]]);
});
});
}, "mathjax-editing");

StackExchange.ifUsing("editor", function () {
StackExchange.using("externalEditor", function () {
StackExchange.using("snippets", function () {
StackExchange.snippets.init();
});
});
}, "code-snippets");

StackExchange.ready(function() {
var channelOptions = {
tags: "".split(" "),
id: "196"
};
initTagRenderer("".split(" "), "".split(" "), channelOptions);

StackExchange.using("externalEditor", function() {
// Have to fire editor after snippets, if snippets enabled
if (StackExchange.settings.snippets.snippetsEnabled) {
StackExchange.using("snippets", function() {
createEditor();
});
}
else {
createEditor();
}
});

function createEditor() {
StackExchange.prepareEditor({
heartbeatType: 'answer',
autoActivateHeartbeat: false,
convertImagesToLinks: false,
noModals: true,
showLowRepImageUploadWarning: true,
reputationToPostImages: null,
bindNavPrevention: true,
postfix: "",
imageUploader: {
brandingHtml: "Powered by u003ca class="icon-imgur-white" href="https://imgur.com/"u003eu003c/au003e",
contentPolicyHtml: "User contributions licensed under u003ca href="https://creativecommons.org/licenses/by-sa/3.0/"u003ecc by-sa 3.0 with attribution requiredu003c/au003e u003ca href="https://stackoverflow.com/legal/content-policy"u003e(content policy)u003c/au003e",
allowUrls: true
},
onDemand: true,
discardSelector: ".discard-answer"
,immediatelyShowMarkdownHelp:true
});


}
});














draft saved

draft discarded


















StackExchange.ready(
function () {
StackExchange.openid.initPostLogin('.new-post-login', 'https%3a%2f%2fcodereview.stackexchange.com%2fquestions%2f203084%2fjquery-image-carousel%23new-answer', 'question_page');
}
);

Post as a guest















Required, but never shown

























1 Answer
1






active

oldest

votes








1 Answer
1






active

oldest

votes









active

oldest

votes






active

oldest

votes









1





+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>








share|improve this answer























  • Feel free to improve my version. This is the reason it is posted here. Thank you!
    – Razvan Zamfir
    Sep 17 '18 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 '18 at 19:45










  • The shuffle(slides); line "randomizes" the slides.
    – Razvan Zamfir
    Sep 18 '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 '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 '18 at 8:45
















1





+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>








share|improve this answer























  • Feel free to improve my version. This is the reason it is posted here. Thank you!
    – Razvan Zamfir
    Sep 17 '18 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 '18 at 19:45










  • The shuffle(slides); line "randomizes" the slides.
    – Razvan Zamfir
    Sep 18 '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 '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 '18 at 8:45














1





+50







1





+50



1




+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>








share|improve this answer














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>








$(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>





$(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>






share|improve this answer














share|improve this answer



share|improve this answer








edited Nov 14 '18 at 16:40

























answered Sep 17 '18 at 16:02









Sᴀᴍ OnᴇᴌᴀSᴀᴍ Onᴇᴌᴀ

9,04362158




9,04362158












  • Feel free to improve my version. This is the reason it is posted here. Thank you!
    – Razvan Zamfir
    Sep 17 '18 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 '18 at 19:45










  • The shuffle(slides); line "randomizes" the slides.
    – Razvan Zamfir
    Sep 18 '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 '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 '18 at 8:45


















  • Feel free to improve my version. This is the reason it is posted here. Thank you!
    – Razvan Zamfir
    Sep 17 '18 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 '18 at 19:45










  • The shuffle(slides); line "randomizes" the slides.
    – Razvan Zamfir
    Sep 18 '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 '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 '18 at 8:45
















Feel free to improve my version. This is the reason it is posted here. Thank you!
– Razvan Zamfir
Sep 17 '18 at 18:45




Feel free to improve my version. This is the reason it is posted here. Thank you!
– Razvan Zamfir
Sep 17 '18 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 '18 at 19: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 '18 at 19:45












The shuffle(slides); line "randomizes" the slides.
– Razvan Zamfir
Sep 18 '18 at 9:58




The shuffle(slides); line "randomizes" the slides.
– Razvan Zamfir
Sep 18 '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 '18 at 18:42




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 '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 '18 at 8:45




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 '18 at 8:45


















draft saved

draft discarded




















































Thanks for contributing an answer to Code Review Stack Exchange!


  • Please be sure to answer the question. Provide details and share your research!

But avoid



  • Asking for help, clarification, or responding to other answers.

  • Making statements based on opinion; back them up with references or personal experience.


Use MathJax to format equations. MathJax reference.


To learn more, see our tips on writing great answers.




draft saved


draft discarded














StackExchange.ready(
function () {
StackExchange.openid.initPostLogin('.new-post-login', 'https%3a%2f%2fcodereview.stackexchange.com%2fquestions%2f203084%2fjquery-image-carousel%23new-answer', 'question_page');
}
);

Post as a guest















Required, but never shown





















































Required, but never shown














Required, but never shown












Required, but never shown







Required, but never shown

































Required, but never shown














Required, but never shown












Required, but never shown







Required, but never shown







Popular posts from this blog

How to reconfigure Docker Trusted Registry 2.x.x to use CEPH FS mount instead of NFS and other traditional...

is 'sed' thread safe

How to make a Squid Proxy server?