0

I'm a beginner in JS, and I can't figure out what's wrong with my attempted slideshow. I understand this may seem like a repeat, and I've found a different, working way, to display a slideshow in JS, but I am simply confused about what is wrong with my code. Every single version of slideshow code I've seen sets all of the image urls in HTML and then hides them and only displays one of them using JS, but why wouldn't mine, with image urls simply set in an array, work?

<img id="sideimg" src="images/image1.jpg" alt="penguin" />
<script>
next();
var next=function(){

    for(var i=0;i<3;i++){
        var slide=document.getElementById("sideimg");
        var slides=["images/image1.jpg","images/image2.jpg","images/image3.jpg"]
        slide.src=slides[i];
    timeOut();
    if(i>=3){
        i=0;
    };
};
var timeOut=function(){
    setTimeout(next,1000);
}

};
</script>
Kat
  • 7
  • 5
  • 1
    Every time you call `next()`, your code is setting `src` to each url in your `slides` array, one after the other. What you probably want to do is have `next` just set the src to the _next_ url – Hamms Aug 18 '18 at 00:20

2 Answers2

2

Order of functions is not right in your sample

You also need i to be defined outside the function and there is no need for the for loop

The following works for me in chrome

<img id="sideimg" src="images/image1.jpg" alt="penguin" />
<script>

var i = 0

var timeOut=function(){
    setTimeout(next,1000);
}

var next=function(){

    var slide=document.getElementById("sideimg");
    var slides=["images/image1.jpg","images/image2.jpg","images/image3.jpg"]
    slide.src=slides[i];
    timeOut();
    i++;
    if(i>=3){
        i=0;
    };
};

next();
</script>

We could also define i within next as follows using IIFE (immediately invoked function expression). It is also better to declare slides and slide outside the function that is invoked every interval.

<img id="sideimg" src="images/image1.jpg" alt="penguin" />
<script>

var timeOut=function(){
    setTimeout(next,1000);
}

var next=function(){
    var i = 0;
    var slides=["images/image1.jpg","images/image2.jpg","images/image3.jpg"];

    var slide=document.getElementById("sideimg");
return function() {
        slide.src=slides[i];
        timeOut();
        i++;
        if(i>=3){
            i=0;
        };
    };
}();

next();
</script>
Raj Sappidi
  • 156
  • 8
  • Although it's better than declaring them in the "for()" loop, `var slide` and `var slides` should be decliared *OUTSIDE* of the function, rather than needlessly being reinitialized each time the function is invoked. Regarding "order of functions", look here: [Understanding hoisting in Javascript](https://scotch.io/tutorials/understanding-hoisting-in-javascript). – paulsm4 Aug 19 '18 at 07:19
  • @paulsm4 you are right! However I was only trying to explain OP what was wrong with their sample with minimal changes. Edited the sample with closure on i to also have closure on slides. – Raj Sappidi Aug 19 '18 at 10:52
1

Try moving the array OUTSIDE of the loop:

var next=function(slides){
   var slide=document.getElementById("sideimg");
   var slides=["images/image1.jpg","images/image2.jpg","images/image3.jpg"]
   for(var i=0;i<3;i++){
      slide.src=slides[i];
      ...
   };

Here is another possibility (WARNING: I haven't actually tried it):

How do I add a delay in a JavaScript loop?

<img id="sideimg" src="images/image1.jpg" alt="penguin" />
<script type="text/javascript">
    var slides=["images/image1.jpg","images/image2.jpg","images/image3.jpg"]
    var slide=document.getElementById("sideimg");
    var i = 0;
    var loop = function () {
      sideimg.src=slides[i];    // Display current image
      i++;
      if (i >= slides.length)
        i = 0;
      setTimeout(function () {  //  call setTimeout when the loop is called
         loop();                //  ..  again which will trigger another 
      }, 1000)
    };
};
</script>
 };
paulsm4
  • 114,292
  • 17
  • 138
  • 190