I’ve cobbled together some JS + jQuery which loops a class through carousel slides and I’ve also added previous/next links which allow manual movement.
I have two issues which I’m unable to resolve mainly due to my lack of experience with JS.
Issue 1. When you click on next or previous the interval timer continues uninterrupted. This means if you click on next 1.9 seconds into the interval, the slide you’ve moved to will display for only 0.1 seconds. Ideally I want the timer to reset on click but have not been able to get this to work.
Issue 2. The terrible verbose code.
I’m usually a stickler for neat concise code, but this is very bloated, I’ve tried moving the variables out of the functions so they can be shared but this seems to break the carousel with the active class skipping either the first or last slide. I’m thinking there must be a much neater more efficient way of pulling all this together.
setInterval(
function() {
var item = $(".slide--active");
var next = item.next(".slide").length == 0 ? $('.slide').first() : item.next(".slide");
var number = $(".current--active");
var succeeding = number.next(".current").length == 0 ? $('.current').first() : number.next(".current");
item.removeClass("slide--active");
next.toggleClass('slide--active');
number.removeClass("current--active");
succeeding.toggleClass('current--active');
},
2000)
$('.next').click(function() {
var item = $(".slide--active");
var next = item.next(".slide").length == 0 ? $('.slide').first() : item.next(".slide");
var number = $(".current--active");
var succeeding = number.next(".current").length == 0 ? $('.current').first() : number.next(".current");
item.removeClass("slide--active");
next.toggleClass('slide--active');
number.removeClass("current--active");
succeeding.toggleClass('current--active');
});
$('.prev').click(function() {
var item = $(".slide--active");
var prev = item.prev(".slide").length == 0 ? $('.slide').last() : item.prev(".slide");
var number = $(".current--active");
var preceding = number.prev(".current").length == 0 ? $('.current').last() : number.prev(".current");
item.removeClass("slide--active");
prev.toggleClass('slide--active');
number.removeClass("current--active");
preceding.toggleClass('current--active');
});
.pagination {
display: flex;
}
.numbers {
padding: 0 1em;
}
.slides {
list-style: none;
display: flex;
}
.slide {
width: 4em;
height: 4em;
background: blue;
}
.slide--active {
background: red;
}
.current {
display: none;
}
.current.current--active {
display: inline;
}
<script src="https://cdnjs.cloudflare.com/ajax/libs/jquery/3.3.1/jquery.min.js"></script>
<div class="pagination">
<a href="#" class="prev">←</a>
<div class="numbers">
<span class="current current--active">1</span>
<span class="current">2</span>
<span class="current">3</span> / <span class="total">3</span>
</div>
<a href="#" class="next">→</a>
</div>
<ul class="slides">
<li class="slide slide--active"></li>
<li class="slide"></li>
<li class="slide"></li>
</ul>
Also on jsfiddle here: https://jsfiddle.net/morgyface/3fbto2qu/52/
3
Answers
This is how you reset interval….
As you pointed out in your post, there is a lot of code here and a lot of it looks duplicated or at least very close to duplicated among the two click handlers and the function passed to
setInterval
.All of the work to determine the next or previous slide based on existing classes is onerous and leads to the clunky code. I think the important conceptual leap to make is to realize is that there is one important variable here: the active index. The active index changes when the prev or next buttons are clicked or when the interval fires. The UI just updates to reflect the new active index.
With that in mind, I would structure the code around the state – what changes, the active index and the identifier of the
setInterval
orsetTimeout
which we will need to track in order to clear it.From there, I would encapsulate the rendering into a single function. This function will do the toggling of the active classes on the slides and the pagination status. The click handlers can each update the state and then call the rendering function. The
setInterval
can do the same. (I ended-up using asetTimeout
, but I think it’s really a matter of taste.)The code I arrived at is:
I have also created a fiddle for reference.
for a
for neat concise code
: