skip to Main Content

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">&larr;</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">&rarr;</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


  1. let variable = setInterval(()=>{})
    clearInterval(variable)
    

    This is how you reset interval….

    Login or Signup to reply.
  2. 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 or setTimeout 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 a setTimeout, but I think it’s really a matter of taste.)

    The code I arrived at is:

    const state = {
      index: $('.slide--active').index(),
      timer: null,
      timerDurationMilliseconds: 2000,
      numSlides: $('.slide').length
    };
    
    const decrementIndex = () => {
      let newIndex = state.index - 1;
      
      if (newIndex < 0) {
        newIndex = state.numSlides - 1;
      }
      
      state.index = newIndex;
    };
    
    const incrementIndex = () => {
      let newIndex = state.index + 1;
      
      if (newIndex >= state.numSlides) {
        newIndex = 0;
      }
      
      state.index = newIndex;
    };
    
    const render = () => {
      $('.current').each(function () {
        const $this = $(this);
        $this.toggleClass('current--active', $this.index() === state.index);
      });
      
      $('.slide').each(function () {
        const $this = $(this);
        $this.toggleClass('slide--active', $this.index() === state.index);
      });
      
      if (state.timer) {
        clearTimeout(state.timer);
      }
      
      state.timer = setTimeout(() => {
        incrementIndex();
        render();
      }, state.timerDurationMilliseconds);
    };
    
    $('.next').on('click', function () {
      incrementIndex();
      render();
    });
    
    $('.prev').on('click', function () {
      decrementIndex();
      render();
    });
    
    render();
    .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">&larr;</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">&rarr;</a>
    </div>
    
    <ul class="slides">
      <li class="slide slide--active"></li>
      <li class="slide"></li>
      <li class="slide"></li>
    </ul>

    I have also created a fiddle for reference.

    Login or Signup to reply.
  3. for a for neat concise code :

    const
      slidesList    = document.querySelectorAll('.slides > li')
    , btNext        = document.querySelector('#next')
    , btPrev        = document.querySelector('#prev')
    , currentNumber = document.querySelector('.numbers > span:first-of-type')
    , slide = 
      { index   : 0
      , max     : slidesList.length
      , refIntv : 0
      , delay   : 2000
      };
    // init...
    movSlide();
    slide.refIntv = setInterval( movSlide, slide.delay, 1);
    document.querySelector('.numbers > span:last-of-type').textContent = slide.max;
    
    btNext.onclick =()=>{ btMove( +1) }
    btPrev.onclick =()=>{ btMove( -1) }
    
    function btMove(val)
      {
      clearInterval(slide.refIntv);
      movSlide( val );
      slide.refIntv = setInterval( movSlide, slide.delay, 1);
      }
    function movSlide( move = 0 )
      {
      slide.index += slide.max + move;
      slide.index %= slide.max;
      currentNumber.textContent = slide.index +1; 
    
      slidesList.forEach( (elm, i) => elm.classList.toggle('slide--active', slide.index===i) );
      }
    .pagination {
      display    : flex;
      }
    .numbers {
      padding : 0.3em 1em 0 1em;
      }
    .slides {
      list-style : none;
      display    : flex;
      column-gap : 5px;
      }
    .slides > li {
      width      : 4em;
      height     : 4em;
      background : blue;
      }
    .slides > li.slide--active {
      background : red;
      }
    <div class="pagination">
      <button id="prev">&larr;</button>
      <div class="numbers">
        <span></span> / <span></span>
      </div>
      <button id="next">&rarr;</button>
    </div>
    
    <ul class="slides">
      <li></li> <li></li> <li></li> <li></li> <li></li> <li></li>
    </ul>

    but if you want to respect a consistency in your interface, it should rather work like this:

    const
      slidesList    = document.querySelectorAll('.slides > li')
    , btNextPrev    = document.querySelectorAll('#next, #prev')
    , divPagination = document.querySelector('div.pagination')
    , currentNumber = document.querySelector('.numbers > span:nth-of-type(1)')
    , currentSign   = document.querySelector('.numbers > span:nth-of-type(2)')
    , slide = 
      { index     : -1
      , max       : slidesList.length
      , direction : +1
      , refIntv   : 0
      , delay     : 2000
      };
    // init...
    movSlide();
    slide.refIntv = setInterval( movSlide, slide.delay);
    document.querySelector('.numbers > span:nth-of-type(2)').textContent = slide.max;
    
    btNextPrev.forEach( btn => btn.onclick =()=>
      {
      clearInterval(slide.refIntv);
      slide.direction = btn.id ==='next' ? +1 : -1;
      movSlide();
      slide.refIntv = setInterval( movSlide, slide.delay);
      divPagination.classList.toggle('goNext', btn.id ==='next' );
      })
    function movSlide()
      {
      slide.index     += slide.max + slide.direction;
      slide.index     %= slide.max;
      currentNumber.textContent = slide.index +1; 
    
      slidesList.forEach( (elm, i) => elm.classList.toggle('slide--active', slide.index===i) );
      }
    .pagination {
      display    : flex;
      }
    .numbers {
      padding : 0.3em 1em 0 1em;
      }
    .slides {
      list-style : none;
      display    : flex;
      column-gap : 5px;
      }
    .slides > li {
      width      : 4em;
      height     : 4em;
      background : blue;
      }
    .slides > li.slide--active {
      background : red;
      }
    div.pagination.goNext       > button#next,
    div.pagination:not(.goNext) > button#prev {
      width  : 3rem;
      }
    <div class="pagination goNext">
      <button id="prev">&larr;</button>
      <div class="numbers">
        <span></span> / <span></span>
      </div>
      <button id="next">&rarr;</button>
    </div>
    
    <ul class="slides">
      <li></li> <li></li> <li></li> <li></li> <li></li> <li></li>
    </ul>
    Login or Signup to reply.
Please signup or login to give your own answer.
Back To Top
Search