Cannot read property 'removeClass' of undefined

Question

I am trying to make an automatic slideshow but this message keeps popping up and I don't understand why.

HTML:

<section id="slideshow">
    <div class="auto-slideshow">
        <img src="img/pic1.jpg" alt="" class="slide show">
        <img src="img/pic2.jpg" alt="" class="slide hide">
        <img src="img/pic3.jpg" alt="" class="slide hide">
    </div>
</section>

The 'show' and 'hide' classes set the display to 'block' and 'none' respectively.

JavaScript:

autoSlideshow();

var mySlides = $('#slideshow .slide');
var slides = [];
mySlides.each(function () {
    slides.push($(this));
});

function autoSlideshow() {
    var index;
    var next;

    mySlides.each(function () {
        if ($(this).hasClass('show')) {
            index = $(this).index();
            next = index+1;

            $(this).removeClass('show').addClass('hide');
            slides[next].removeClass('hide').addClass('show');

            console.log('The index is: '+index);
            console.log('The next one is: '+next);
        };
    });
    setInterval(autoSlideshow, 3000);
};

Any advice or correction is much appreciated.

Answer 1

You get the error because you get an out of bound index. As your array contains 3 elements and the index start at 0, array[3] will be out of bounds.

Apart from that, $('#slideshow .slide') already gives you an array of elements, so no need to define an extra array for it. Furthermore, don't use the setInterval inside your function as this will eventually get your script to crash as it gets called over and over again without clearing it. Inside your slideshow function, don't call the each as this will do the update on every slide in one go. What you want to do is when your function gets called, hide all slides and show only the current index.

When your index reaches the end of the array, you reset it to 0 to start over.

The .hide class seems redundant, just add a display: none to your .slide.

Something like this will do:

var mySlides = $('#slideshow .slide');
// whats the start index
var slideIndex = $('#slideshow .show').index();
// how many slides
var noOfSlides = mySlides.length;

function autoSlideshow() {
  // remove the 'show' class of all slides
  mySlides.removeClass('show');

  // add 'show' class to only the slide with the slideIndex index
  mySlides.eq(slideIndex).addClass('show');

  // update index
  slideIndex++;

  // restart at 0
  if (slideIndex >= noOfSlides)
    slideIndex = 0;

};

autoSlideshow();
setInterval(autoSlideshow, 3000);

.slide {
  display: none;
}
.show {
  display: block;
}

<script src="https://ajax.googleapis.com/ajax/libs/jquery/1.11.0/jquery.min.js"></script>
<section id="slideshow">
  <div class="auto-slideshow">
    <img src="http://placehold.it/350x150/333" alt="" class="slide show">
    <img src="http://placehold.it/350x150/555" alt="" class="slide">
    <img src="http://placehold.it/350x150/111" alt="" class="slide">
  </div>
</section>

Answer 2

Pretty close, just a couple things.

You're iterating over all the slides, updating all of them

function autoSlideshow() {
  mySlides.each(function () { //code snipped }
}

This is looping over all the slides, each iteration sets the next slide to visible, so when the loop finishes you're right back where you started.

You're adding a new timer each time you call the function

function autoSlideshow() {
  //code snipped
  setInterval(autoSlideshow, 3000);
};

Every time you call this function, you add another timer that calls it again. You only need to use setInterval() once.

CSS updates Having a class set to display: none; requires you to remove that class every time you update the slides. A better approach would be to have display: none; be the default property for the images in the slideshow. Then you only need to add the show class and not worry about removing the hide class.

Updated Code:

JavaScript

$(document).ready(function() { //wait until the DOM is ready...
  var mySlides = $('#slideshow .slide');
  var slides = [];
  var index = 0;

  mySlides.each(function () {
    slides.push($(this));
  });

  function autoSlideshow() {
    index++;
    var nextSlide = index%slides.length;  //the mod function makes sure you stay within the bounds of the array
    $('.show').removeClass('show'); //remove the show class from the existing slide
    slides[nextSlide].addClass('show'); //add the show class to the next slide
  };
  setInterval(autoSlideshow, 3000);
})

HTML

<section id="slideshow">
  <div class="auto-slideshow">
    <img src="https://unsplash.it/200/300" alt="" class="slide show">
    <img src="https://unsplash.it/200/301" alt="" class="slide">
    <img src="https://unsplash.it/200/302" alt="" class="slide">
  </div>
</section>

CSS

#slideshow img { display: none; }
#slideshow img.show { display: block; }

See it working in this JS Fiddle: https://jsfiddle.net/igor_9000/9mhujoa9/

Hope that helps!

Answer 3

First you should call your autoSlideshow() after you define mySlides. Then re-initialize value of next when it gets out of bound. Best way to call autoSlideshow will be to take it out of the method:

function autoSlideshow() {
    var index;
    var next;

    mySlides.each(function() {
        if ($(this).hasClass('show')) {
            index = $(this).index();
            next = index + 1;
            next = next >= mySlides.length ? 0 : next;

            //DO WHAT YOU WERE DOING
        };
    });

};

setInterval(autoSlideshow, 3000);

Answer 4

Using the HTML you provided, you can assume that the slides variable has 3 elements in it. Now, when iterating over mySlides, you assign the index to a variable and use that value + 1 to access an element of the slides array. What happens if you are currently on the 3rd item of the mySlides array? The index would be two and next would be set to 3. Since slides has only 3 values and thus can only reach slides[2], the code is trying to access a currently undefined index of slides.