JavaScript - Image slider buttons not working

Question

I'm learning JavaScript. And now I want to make a typical image slider that when the page loads automatically starts, and if one of 2 buttons (forward and backward) are clicked it stops the automatic slider and just goes to where we click the buttons.

But the code that I'm testing doesn't have the buttons working. I have even used jQuery attempting to make it work, but no success. I think that I have made a mess, but this is the code:

$(function() {
  var slideIndex = 0;
  var i;
  var s = document.getElementsByClassName("slide");
  var stopslider = false;
  slider();
  
  $(".change[n]").click(function(){
    var slideIndex = 1;
    var stopslider = true;
    $( "slide1" ).removeClass( "slide" ).addClass( "1slide" );
    $( "slide2" ).removeClass( "slide" ).addClass( "1slide" );
    $( "slide3" ).removeClass( "slide" ).addClass( "1slide" );
    $( "slide4" ).removeClass( "slide" ).addClass( "1slide" );
    var s = document.getElementsByClassName("1slide");
    slideslide1Index += n;
    if (n > s.length) {
      slideIndex = 1
    } 
    if (n < 1) {
      slideIndex = s.length
    } ;
    for (i = 0; i < s.length; i++) {
      s[i].style.display = "none"; 
    }
    s[slideIndex-1].style.display = "block"; 
  });
  
  function slider() {
    for (i = 0; i < s.length; i++) {
      s[i].style.display = "none"; 
    }
    slideIndex++;
    if (slideIndex > s.length) {
      slideIndex = 1
    }
    s[slideIndex-1].style.display = "block";
    setTimeout(slider, 4000);
    if (stopslider= true) {
      clearTimeout(slider);
    } 
  }
});

myfunkyside · Accepted Answer

I had another look at my first answer and realized that may entirely not be what you're looking for.
So here I'll try to stay as true to your original code as possible.

See the comments below the code snippet for explanation.

window.onload = function() {
  var slideIndex = 0;
  var stopslider = false;
  slider();
  
  function change(n) {
    stopslider = true;
    var s = document.getElementsByClassName("slide");
    slideIndex += n;
    if (slideIndex > s.length) {
      slideIndex = 1;
    } else if (slideIndex < 1) {
      slideIndex = s.length;
    }
    for (var i=0; i s.length) {
        slideIndex = 1;
      }
      s[slideIndex-1].style.display = "block";
      setTimeout(slider, 4000);
    }
  }
  
//CLICK-HANDLERS-------------------------
  document.getElementById("previous").onclick = function(){change(-1);};
  document.getElementById("latter").onclick = function(){change(+1);};
};

jsfiddle: https://jsfiddle.net/Lkeaejkn/

Since you stated that originally you were coding in pure JS, I reverted all the jQuery back.
So $(function() { becomes window.onload = function() {.
I removed global variable var s = document.getElementsByClassName("slide");. You could use it, but since you also already declare it inside one function, and you generally want to keep your global variables to a minimum (link1, link2), I chose to remove the global one.
I also removed var i;, same reason. It's better to declare them inside the for-loop.
Inside function change(n), I removed var slideIndex = 1; because this would override/interfere with the global variable var slideIndex you had already declared at the start of the script.
Unfortunately, this variable has to stay global, otherwise you would reset the value every time you call the function change(n), and the slide would never change. This is one of the reasons I created my first answer, to avoid global variables. If you're interested, check that one out.
1. I removed var from var stopslider = true;. The var creates a new variable that is only visible inside the function (so not global), but in this case we do need to reference the global var stopslider, because that is the variable that is checked inside the function slider() to determine if the auto-slider should be stopped.
2. I removed all the $( "slide1" ).removeClass( "slide" ).addClass( "1slide" );. I don't know what you were trying there, but it's not necessary. (Also, jQuery selectors for id need a # in front of the id, so: $("#slide1")).
  I also went ahead and removed all the ids from the slides in HTML, because they are not necessary and it can save you a lot of pain when you want to insert more slides later on.
3. I also removed the 1 from var s = document.getElementsByClassName("1slide");, don't know what you were doing there either.
  I also moved class="slide" from the s to the s, because otherwise the images would be hidden, but the s of all images would still be visible, which is not what your want.
  Your actually don't need the s for the images or the slider itself, but you may have a specific purpose for them, so I'll leave them in.

In all your if-clauses you check for n: if (n > s.length) {, but you should check for slideIndex. Remember, n only ever has a value of either -1 or +1. Global slideIndex holds the value of the current slide.

I combined your two if-clauses into an if-else: } else if (slideIndex < 1) {. Because only ever one of the two clauses can be true, it inefficient to run them both each time change(n) is called. And this also directly shows (to the programmer who reads the code) that these two clauses belong together, are part of one bigger check.

You had a typo: } ;, I removed it.

As I said earlier it's better to declare i inside the for-loop, so that becomes: for (var i=0; i (same in function slider()).


Inside function slider(), because I removed the global var s = document.getElementsByClassName("slide");, I had to add that line at the top of this function, just like in change(n).
In the if-clause condition I changed if (stopslider= true) { to if (stopslider == true) {.

Because = is already used to assign value, in programming we use == (double =) to check if two values are equal to each other.
In order to be able to clear a timeout, you unfortunately first need to store a global reference to that timer when you set it, so: timer = setTimeout(slider,4000);, where timer is a global var that you set at the start of your script.

Fortunately, we don't actually need to clear the timer. Because setTimeout() only runs once and after that just stops, we just need to not set it again.

In order to do that, just put all the code from function slider() inside the if-clause that you used around clearTimeout(), only changing true to false: if (stopslider == false) {.

(If you put it only around setTimeout(), the slider will still change automatically one more time after you clicked a button for the first time.)



I changed the src of your images, just so we can actually see them change.



Code snippet using data- attribute:

This snippet is based on the previous snippet, with a few changes.



window.onload = function() {
  var slideIndex = 0;
  var stopslider = false;
  slider();
  
  function slider() {
    if (stopslider == false) {
      var s = document.getElementsByClassName("slide");
      for (var i=0; i s.length) {
        slideIndex = 1;
      }
      s[slideIndex-1].style.display = "block";
      setTimeout(slider, 4000);
    }
  }
  
//CLICK-HANDLERS-------------------------
  (function() {
    var c = document.getElementsByClassName("change");
    for (var i=0; i s.length) {
          slideIndex = 1;
        } else if (slideIndex < 1) {
          slideIndex = s.length;
        }
        for (var i=0; i











jsfiddle: https://jsfiddle.net/Lkeaejkn/2/


in HTML, I added data-n="-1" and data-n="+1" to the respective buttons, and removed the ids from the buttons as they are no longer necessary. Read about the data-* attribute
In JS, I removed the old id-based click-handlers, and instead changed the whole function change(n) to an anonymous click-handler for the buttons.
I wrapped click-handlers in an self-executing anonymous function to prevent any more global variables: (function() { ... })();.
Then I use the same principle as you use for the slides, but now to iterate over the buttons.

And then for each button, I bind the actual click-handler: c[i].onclick = function() {.
Inside the click-handler is use var n = parseInt(this.getAttribute("data-n")); to get the n-value from the respective button.

Because the data from the data-n attribute is in text format, parseInt() is needed to convert the text value to an integer.

JavaScript - Image slider buttons not working

Answers (2)

Code snippet using `data-` attribute:

PURE JS:

JQUERY:

Related Questions