0

Sorry for the noob question, but I have been struggling for a couple of hours here. I want to make an automatic slider with buttons, but the function showSlides doesn't inherit the slideIndex value from currentSlide. Here's my code:

var slideIndex = 0;
setTimeout(showSlides, 3000);
  var i;
  var n;
  var slides = document.getElementsByClassName("mySlides");
  
function currentSlide(slideIndex = n) {
  var i;
  var slides = document.getElementsByClassName("mySlides");
  
  for (i = 0; i < slides.length; i++) {
      slides[i].classList.remove('active')
  }
  console.log(slideIndex)
  slides[slideIndex].classList.add('active')
}
function showSlides() {
  var i;
  var slides = document.getElementsByClassName("mySlides");
  
  for (i = 0; i < slides.length; i++) {
      slides[i].classList.remove('active')
  }
    if(slideIndex == slides.length-1){
        slideIndex = 0
        } else {
        slideIndex = slideIndex+1
        }
  console.log(slideIndex)
  slides[slideIndex].classList.add('active')

  setTimeout(showSlides, 3000);
}
  • The function `currentSlide` does not get invoked at all in the above posted code, thus this function can not effect anything. And *inheritance* is yet another thing/matter with most probably not the slightest connection to the OP's problem. – Peter Seliger Jun 09 '21 at 17:18

2 Answers2

2

Your Global slideIndex is all you need to keep track of. Your argument of the same name "hides" the Global one for the duration of that function, and is not necessary here.

Also, you should avoid .getElementsByClassName() as well as re-scanning the DOM for the same elements over and over.

Additionally, since you need to loop through all the slides and hide them more than once, break that code out into its own function so you don't have the same code in more than one place.

Lastly, your currentSlide function isn't even being called and doesn't need to be so it can be removed.

var slideIndex = 0;
var i = null;
var n = null;

// You only need to find the slides once, not in each function
var slides = document.querySelectorAll(".mySlides");

setTimeout(showSlides, 1000);

function showSlides() {
  hideAllSlides();
  if(slideIndex == slides.length-1){
    slideIndex = 0;
  } else {
    slideIndex += 1;
  }
  console.log(slideIndex)
  slides[slideIndex].classList.add('active')

  setTimeout(showSlides, 1000);
}

function hideAllSlides(){
  // Use .forEach(), it's much simpler
  slides.forEach(function(slide){
    slide.classList.remove("active");
  });
}
.active { color:red; }
<div class="mySlides active">slide</div>
<div class="mySlides">slide</div>
<div class="mySlides">slide</div>
<div class="mySlides">slide</div>
Scott Marcus
  • 64,069
  • 6
  • 49
  • 71
1

In the declaration for currentSlide you're creating a local slideIndex which is not the same as the global slideIndex even though they have the same name. This local slideIndex is scoped to the currentSlide function only. You don't want to do this.

function currentSlide(n) { // not creating the local slideIndex
  var i;
  slideIndex = n; // slideIndex here is global
  var slides = document.getElementsByClassName("mySlides");
  
  for (i = 0; i < slides.length; i++) {
      slides[i].classList.remove('active')
  }
  slides[slideIndex].classList.add('active')
}
Mordred
  • 3,734
  • 3
  • 36
  • 55