Multiple slideshows on one page makes the first one not work anymore

First of all I know this question’s been asked before, the answer wasn’t solving my problem so I’d like to ask it again here:

I’m using the code blocks to create more customized slideshows. I need more than one slideshow on my page and the second one stops the first one from working. The slideshow is here but clicking the arrows won’t do anything. I slightly understand what the problem is but can’t fix it. Here is the link to the page which contains 2 slideshows (the code of each slideshow is working as expected when there’s only one slideshow):

https://www.guiyingzhu.com/need-help

password: squarespace

1st slideshow:

<!DOCTYPE html>
<html>
<head>
<meta name="viewport" content="width=device-width, initial-scale=1">
<style>
* {box-sizing: border-box}
.mySlides {display: none}
img {vertical-align: middle}

/* Slideshow container */
.slideshow-container {
  max-width: 1000px;
  position: relative;
  margin: auto;
  box-shadow: 0 -1px 6px 0.5px #F0F6FF
}

/* Next & previous buttons */
.prev, .next {
  cursor: pointer;
  position: absolute;
  top: 50%;
  outline: none;
  color: #fff !important;
  z-index: 999;
  font-size: 30px;
  line-height: 40px;
  width: 50px;
  margin-top: -30px;
  background-color:rgba(0,0,0,.12);
  display: inline-block;
  padding: 15px;
  transition: 0.6s ease} 
/* Position the "next button" to the right */
.next {
  right: 0;
  border-radius: 3px 0 0 3px;
}

/* On hover, add a black background color with a little bit see-through */
.prev:hover, .next:hover {
  background-color: rgba(0,0,0,.22);
}

/* The dots/bullets/indicators */
.dot {
  cursor: pointer;
  height: 15px;
  width: 15px;
  margin: 0 2px;
  background-color: #E8EBEF;
  border-radius: 50%;
  display: inline-block;
  transition: background-color 0.6s ease;
}

.active, .dot:hover {
  background-color: #91A8D0;
}

</style>
</head>
<body>

<div class="slideshow-container">

<div class="mySlides fade">
  <img src="https://static1.squarespace.com/static/59ed8a809f07f53bd978da62/t/5df6c3a40027c56344c75ca5/1576453034160/Popcorn_Iterative+Feedback_20192_1.png" style="width:100%">
</div>

<div class="mySlides fade">
  <img src="https://static1.squarespace.com/static/59ed8a809f07f53bd978da62/t/5df6c3ae1bd90e41f9535bb4/1576453045483/Popcorn_Iterative+Feedback_20192_2.png" style="width:100%">
</div>

<a class="prev" onclick="plusSlides(-1)">&#10094;</a>
<a class="next" onclick="plusSlides(1)">&#10095;</a>

</div>
<br>

<div style="text-align:center">
  <span class="dot" onclick="currentSlide(1)"></span> 
  <span class="dot" onclick="currentSlide(2)"></span>
</div>

<script>
var slideIndex = 1;
showSlides(slideIndex);

function plusSlides(n) {
  showSlides(slideIndex += n);
}

function currentSlide(n) {
  showSlides(slideIndex = n);
}

function showSlides(n) {
  var i;
  var slides = document.getElementsByClassName("mySlides");
  var dots = document.getElementsByClassName("dot");
  if (n > slides.length) {slideIndex = 1}    
  if (n < 1) {slideIndex = slides.length}
  for (i = 0; i < slides.length; i++) {
      slides[i].style.display = "none";  
  }
  for (i = 0; i < dots.length; i++) {
      dots[i].className = dots[i].className.replace(" active", "");
  }
  slides[slideIndex-1].style.display = "block";  
  dots[slideIndex-1].className += " active";
}
</script>

</body>
</html> 

2nd slideshow:

<!DOCTYPE html>
<html>
<head>
<meta name="viewport" content="width=device-width, initial-scale=1">
<style>
* {box-sizing: border-box}
.mySlides2 {display: none}
img {vertical-align: middle}

/* Slideshow container */
.slideshow-container2 {
  max-width: 1000px;
  position: relative;
  box-shadow: 0 -1px 6px 0.5px #F0F6FF
  margin:0;
  text-align:center; 
}

/* Next & previous buttons */
.prev2{
  position: relative;
  left: -300px;
  top: -350px;
  cursor: pointer;
  outline: none;
  color: #fff !important;
  z-index: 999;
  font-size: 30px;
  line-height: 40px;
  width: 50px;
  margin-top: -30px;
  background-color:rgba(0,0,0,.12);
  display: inline-block;
  padding: 15px;
  transition: 0.6s ease}

.next2{
  position: relative;
  left: 300px;
  top: -350px;
  cursor: pointer;
  outline: none;
  color: #fff !important;
  z-index: 999;
  font-size: 30px;
  line-height: 40px;
  width: 50px;
  margin-top: -30px;
  background-color:rgba(0,0,0,.12);
  display: inline-block;
  padding: 15px;
  transition: 0.6s ease} 

/* Position the "next button" to the right */
.next2 {
  right: 0;
  border-radius: 3px 0 0 3px;
}

/* On hover, add a black background color with a little bit see-through */
.prev2:hover, .next2:hover {
  background-color: rgba(0,0,0,.22);
}

/* The dots/bullets/indicators */
.dot2 {
  position: relative;
  top: -60px;
  cursor: pointer;
  height: 15px;
  width: 15px;
  margin: 0 2px;
  background-color: #E8EBEF;
  border-radius: 50%;
  display: inline-block;
  transition: background-color 0.6s ease;
}

.active2, .dot2:hover {
  background-color: #91A8D0;
}

</style>

</head>

<body>

<div class="slideshow-container2">

<div class="mySlides2 fade">
  <img src="https://static1.squarespace.com/static/5dfc597bce57214c46fd4479/t/5dff19d25a4eb35ea7791a35/1576999428339/gif%2B0.gif" style="width:43%">
  <div class="text"><p><p>Sign In & Explore the Home Page</div>
</div>

<div class="mySlides2 fade">
  <img src="https://static1.squarespace.com/static/5dfc597bce57214c46fd4479/t/5dff1a7248705800ca136acd/1576999585644/gif%2B1.gif" style="width:43%">
  <div class="text"><p><p>Discover a Moive</div>
</div>

<a class="prev2" onclick="plusSlides(-1)">&#10094;</a>
<a class="next2" onclick="plusSlides(1)">&#10095;</a>

</div>
<br>

<div style="text-align:center">
  <span class="dot2" onclick="currentSlide(1)"></span> 
  <span class="dot2" onclick="currentSlide(2)"></span>
</div>

<script>
var slideIndex = 1;
showSlides(slideIndex);

function plusSlides(n) {
  showSlides(slideIndex += n);
}

function currentSlide(n) {
  showSlides(slideIndex = n);
}

function showSlides(n) {
  var i;
  var slides = document.getElementsByClassName("mySlides2");
  var dots = document.getElementsByClassName("dot2");
  if (n > slides.length) {slideIndex = 1}    
  if (n < 1) {slideIndex = slides.length}
  for (i = 0; i < slides.length; i++) {
      slides[i].style.display = "none";  
  }
  for (i = 0; i < dots.length; i++) {
      dots[i].className = dots[i].className.replace(" active2", "");
  }
  slides[slideIndex-1].style.display = "block";  
  dots[slideIndex-1].className += " active2";
}
</script>

</body>
</html>

What I see happening on your test page, is that the first slideshow causes the second slideshow to trigger instead.

Looking at your HTML code, you have invalid HTML code, which makes it very difficult for JavaScript to correctly work with the HTML code.

Using the standard HTML validator, the first problem is on the xmlns line.

Error : Attribute xmlns:og not allowed here.

<html xmlns:og="http://opengraphprotocol.org/schema/" xmlns:fb="http://www.facebook.com/2008/fbml" lang="en-US"  >

That line doesn’t have a problem itself, but is a symptom of a larger issue. An appropriate doctype is needed that supports xmlns:og which is the RDFa doctype.

<!DOCTYPE html PUBLIC "-//W3C//DTD XHTML+RDFa 1.0//EN" "http://www.w3.org/MarkUp/DTD/xhtml-rdfa-1.dtd">

No Character encoding declared at document level

The following is not accepted by the HTML validator

<meta charset="utf-8" />

What is needed instead is:

<meta http-equiv="Content-Type" content="text/html; charset=utf-8"/>

I recommend that you carry on with fixing all of the HTML problems with the page, or create a simpler version of the page that has no HTML errors, before moving on with JavaScript support.

Hi Paul, thanks for your attention! It’s interesting because the code you mentioned is not from me lol They might belong to the Squarespace template.

But you’re right I just realized that the first slideshow causes the second slideshow to trigger instead…

Here’re some solutions I found and tried but didn’t work out:

If you are more interested in the minimal work to just get it working, both slideshows refer to the same function, and that function has no idea about which slideshow it should control.

Of the two options, one being to rename separate copies of the function, and the other being to pass information to the function about which slideshow to affect, the second option is by the more preferable.

That’s the part I’m really struggling with :exploding_head: I tried to assign a different name to the slideshows and refer to them differently in the script, like this

  #example#
<a class="prev1" onclick="plusDivs(-1, 0)">&#10094;</a>
<a class="next1" onclick="plusDivs(1, 0)">&#10095;</a>

  #example#
var slideIndex = [1];
var slideId = ["mySlides1"]
showDivs(1, 0);
  #example#
<a class="prev2" onclick="plusDivs(-1, 1)">&#10094;</a>
<a class="next2" onclick="plusDivs(1, 1)">&#10095;</a>

  #example#
var slideIndex = [1];
var slideId = ["mySlides2"]
showDivs(1, 1);

But it still doesn’t work, and sometimes one of the slideshow is gone (the function can’t find the slideshow I tried to refer to).

It’s no good just changing the slideshow name you would need to rename everything in the second script so that you have new function names and different variables and different classes to target in the slideshow html. The onclick handler would then be changed to the new function name.

You can’t use the same code twice unless it’s all unique or unless as Paul stated above you rewrite the code in a way that can handle multiple slideshows. This would of course be more complex than the current routine :).

I’m sure @Paul_Wilkins will prompt you in the right direction once you have grasped the problem :slight_smile:

1 Like

Best results are obtained by ensuring that the functions don’t reach out for things, and instead work with information that’s given to them.

With the first slideshow example, the showSlides function reaches out to get elements with a classname of mySlides and of dot, but that breaks when there’s more than one slideshow.

Fixing indents

While setting my code editor to use an indent of four spaces, I note that earlier HTML code in the first slideshow example causes bad indent to occur.

The transition is the first thing that needs to be fixed;

/*          transition: 0.6s ease} */
            transition: 0.6s ease;
        } 

Removing inline event handlers

But I’m getting ahead of myself here. The inline onclick event handlers are where the problems begin, and must be dealt with first. At least for my own sanity.

Inline event handlers are blind to where they were invoked. The better technique is to use JavaScript to add an event handler, so that when it is invoked, it knows the element that invoked it.

We can now start by removing the inline event handlers on the dots:


[temporary interruption] Oh, there’s a thread update.

Thanks @PaulOB - I’m working on a step by step guide about getting them to work together.

2 Likes

Removing inline event handlers

The inline event handlers are removed by removing the inline event handlers on the HTML code.

Before:

        <span class="dot" onclick="currentSlide(1)"></span> 
        <span class="dot" onclick="currentSlide(2)"></span>

After:

        <span class="dot"></span> 
        <span class="dot"></span>

That doesn’t leave the code in a working state though, so next we’ll add JavaScript event handlers.

Add JavaScript event handlers

For the JavaScript event handlers we’ll get each of the dots, and add an event handler to each of them.

        const dots = document.querySelectorAll(".dot");
        dots[0].addEventListener(function dotClickHandler(evt) {
            showSlides(1);
        });
        dots[1].addEventListener(function dotClickHandler(evt) {
            showSlides(2);
        });

The slideshow now works after that change, but the code’s not good, as it has some ugly duplication in there. Let’s remove that duplication.

Remove duplication

The event functions we added are nearly identical. When we use a forEach method we can easily get the index of each dot, which lets us easily remove that duplication.

Instead of showing before and after, which takes up a lot of space, I’ll comment out the old code instead, which should help to make it easier to see how the changes occur.

        dots.forEach(function addDotHandlers(dot, index) {
            // dots[0].addEventListener("click", function dotClickHandler(evt) {
            dot.addEventListener("click", function dotClickHandler(evt) {
                // currentSlide(1);
                currentSlide(index + 1);
            });
        });
        // dots[1].addEventListener("click", function dotClickHandler(evt) {
        //     currentSlide(2);
        // });

Updating the prev/next events

The HTML code for the prev/next buttons can have its inline events removed too.

    <!--<a class="prev" onclick="plusSlides(-1)">&#10094;</a>-->
        <a class="prev">&#10094;</a>
    <!--<a class="next" onclick="plusSlides(1)">&#10095;</a>-->
        <a class="next">&#10095;</a>
        const prev = document.querySelector(".prev");
        prev.addEventListener("click", function prevClickHandler(evt) {
            plusSlides(-1);
        });
        const next = document.querySelector(".next");
        next.addEventListener("click", function nextClickHandler(evt) {
            plusSlides(1);
        });

We can now remove duplication from there. In the clickHandler we can access the element that was clicked. if it has a classname of prev we can go -1, and if it has a classname of next we can go +1.

We can do this slowly, first updating the functions so that they are identical and still working, then combine the two sets of code.

        const prev = document.querySelector(".prev");
        prev.addEventListener("click", function prevClickHandler(evt) {
            const el = evt.target;
            if (el.classList.contains("prev")) {
                plusSlides(-1);
            }
            if (el.classList.contains("next")) {
                plusSlides(1);
            }
        });
        const next = document.querySelector(".next");
        next.addEventListener("click", function nextClickHandler(evt) {
            const el = evt.target;
            if (el.classList.contains("prev")) {
                plusSlides(-1);
            }
            if (el.classList.contains("next")) {
                plusSlides(1);
            }
        });

Loop through prev and next buttons

We can now combine the two slide handler functions together, by looping through a collection of prev and next elements.

        // const prev = document.querySelector(".prev");
        const slideButtons = document.querySelectorAll(".prev, .next");
        slideButtons.forEach(function addSlideHandlers(slide) {
            // prev.addEventListener("click", function prevClickHandler(evt) {
            slide.addEventListener("click", function slideClickHandler(evt) {
                const el = evt.target;
                if (el.classList.contains("prev")) {
                    plusSlides(-1);
                }
                if (el.classList.contains("next")) {
                    plusSlides(1);
                }
            });
        });
        // const next = document.querySelector(".next");
        // next.addEventListener("click", function nextClickHandler(evt) {
        //     const el = evt.target;
        //     if (el.classList.contains("prev")) {
        //         plusSlides(-1);
        //     }
        //     if (el.classList.contains("next")) {
        //         plusSlides(1);
        //     }
        // });

That is the inline event attributes removed, moving what they do in to proper JavaScript handler functions. Those will become useful as we improve the code so that it can work with multiple sliders, which is what we’ll look at in the next post.

3 Likes

The easiest way to test how this goes with multiple slideshows, is to put two identical slideshows on the page.

    <div class="slideshow-container">
        
        <div class="mySlides fade">
            <img src="https://static1.squarespace.com/static/59ed8a809f07f53bd978da62/t/5df6c3a40027c56344c75ca5/1576453034160/Popcorn_Iterative+Feedback_20192_1.png" style="width:100%">
        </div>
        
        <div class="mySlides fade">
            <img src="https://static1.squarespace.com/static/59ed8a809f07f53bd978da62/t/5df6c3ae1bd90e41f9535bb4/1576453045483/Popcorn_Iterative+Feedback_20192_2.png" style="width:100%">
        </div>
        
        <a class="prev">&#10094;</a>
        <a class="next">&#10095;</a>
        
    </div>
    <br>
    
    <div style="text-align:center">
        <span class="dot"></span> 
        <span class="dot"></span>
    </div>
    
    <div class="slideshow-container">
        
        <div class="mySlides fade">
            <img src="https://static1.squarespace.com/static/59ed8a809f07f53bd978da62/t/5df6c3a40027c56344c75ca5/1576453034160/Popcorn_Iterative+Feedback_20192_1.png" style="width:100%">
        </div>
        
        <div class="mySlides fade">
            <img src="https://static1.squarespace.com/static/59ed8a809f07f53bd978da62/t/5df6c3ae1bd90e41f9535bb4/1576453045483/Popcorn_Iterative+Feedback_20192_2.png" style="width:100%">
        </div>
        
        <a class="prev">&#10094;</a>
        <a class="next">&#10095;</a>
        
    </div>
    <br>
    
    <div style="text-align:center">
        <span class="dot"></span> 
        <span class="dot"></span>
    </div>    

We’ll know that the scripting code works when the slide buttons and the dots each control their own respective slideshow, with no errors in the browser console.

Why aren’t both slideshows showing?

For some reason both slideshows aren’t showing when the page loads. Currently the showSlides function affects all elements called mySlides, which was lazy and is now wrong. Instead of doing that it needs to affect only the slides inside of the appropriate slideshow container.

        const container = document.querySelector(".slideshow-container");
        var slideIndex = 1;
        // showSlides(slideIndex);
        showSlides(container, slideIndex);
        ...
        function showSlides(container, n) {
            ...
        }

While looking at this other code, I’ll add some things to a TODO list to come back to later.

TODO:

  • Rename n to a more meaningful name

plusSlides and currentSlide also need to pass the container element through:

        // function plusSlides(n) {
        function plusSlides(container, n) {
            // showSlides(slideIndex += n);
            showSlides(container, slideIndex += n);
        }
        
        // function currentSlide(n) {
        function currentSlide(container, n) {
            // showSlides(slideIndex = n);
            showSlides(container, slideIndex = n);
        }

TODO:

  • Rename n to a more meaningful name
  • slideIndex shouldn’t be reassigned in the parameters of a function call

Those plusSlides/currentSlides functions are also used in the event handlers, which need to be updated too.

                if (el.classList.contains("prev")) {
                    // plusSlides(-1);
                    plusSlides(container, -1);
                }
                if (el.classList.contains("next")) {
                    // plusSlides(1);
                    plusSlides(container, 1);
                }
         ...
            dot.addEventListener("click", function (evt) {
                // currentSlide(index + 1);
                currentSlide(container, index + 1);
            });

That’s a lot of places where container had to be assigned. We can remove the need for some of those by making our code a bit smarter, and find the container itself. That we can do later in the event handler functions.

TODO:

  • Rename n to a more meaningful name
  • slideIndex shouldn’t be reassigned in the parameters of a function call
  • Let handlers find the container by walking up the DOM to the container

Get slides and dots from the container

Now that the appropriate container is available, we can get the slides that are contained inside of that container:

            // var slides = document.getElementsByClassName("mySlides");
            var slides = container.querySelector(".mySlides");

And the dots are in the div just after the container. That’s a complex-enough task though to assign to a separate function:

            // var dots = document.getElementsByClassName("dot");
            var dots = getNextDots(container);

The getDots function needs to walk forward through the DOM until it finds the next div section, and give any dot elements that are in there.

        function getNextDots(container) {
            let div = container.nextElementSibling;
            while (div && div.querySelectorAll(".dot").length === 0) {
                div = div.nextElementSibling;
            }
            if (div) {
                return div.querySelectorAll(".dot");
            }
        }

I should now be able to pass multiple container elements, and have them each update on the page when it loads.

        // const container = document.querySelector(".slideshow-container");
        const containers = document.querySelectorAll(".slideshow-container");
        containers.forEach(function showFirstSlide(container) {
            var slideIndex = 1;
            showSlides(container, slideIndex);
        });

The work’s not over yet. We need to also get the slide buttons and the dots working. First the slide buttons.

Get slide buttons working with multiple slideshows

We can now take care of one of the TODO items on our list, of letting an event handler find the container.

What with adding more code to the event handler, this is now the right time to move that code out to a separate function. Event handlers shouldn’t do the work themself, and should just pass on work to another function.

        function addSlideHandler(el) {
            if (el.classList.contains("prev")) {
                plusSlides(container, -1);
            }
            if (el.classList.contains("next")) {
                plusSlides(container, 1);
            }    
        }
...
            slide.addEventListener("click", function slideClickHandler(evt) {
                const el = evt.target;
                // if (el.classList.contains("prev")) {
                //     plusSlides(container, -1);
                // }
                // if (el.classList.contains("next")) {
                //     plusSlides(container, 1);
                // }    
                addSlideHandler(el);
            });

In the addSlideHandler function, we can get the container from reference of the el element, using a similar system as we did for getting the dots.

        function getContainer(el) {
            let container = el.parentNode;
            while (el && !el.classList.contains("slideshow-container")) {
                el = el.parentNode;
            }
            return el;
        }

And the slideshow prev and next slide buttons now work on all slideshows, all but for one problem. The slideIndex isn’t separate for each slideshow yet, which I’ll add to the TODO list.

TODO:

  • Rename n to a more meaningful name
  • slideIndex shouldn’t be reassigned in the parameters of a function call
  • Let handlers find the container by walking up the DOM to the container
  • Separate slideIndex for each slideshow

Get dots to work with each slideshow

We can use a similar process to get the dots working. First move the event listener contents out to a separate function:

            dot.addEventListener("click", function dotClickHandler(evt) {
                const dot = evt.target;
                addDotHandler(dot, index);
            });

Then get the container from the currently active dot:

        function addDotHandler(dot, index) {
            const container = getPrevContainer(dot);
            currentSlide(container, index + 1);
        }

And create a getPrevContainer function that goes to the dot parent, and walks up previous elements until it finds the container:

        function getPrevContainer(dot) {
            el = dot.parentNode;
            while (el && !el.classList.contains("slideshow-container")) {
                el = el.previousElementSibling;
            }
            return el;
        }

That works with the first container, but not the second. And that’s because we are looping through all of the dots, on all of the containers.

Instead of that, we just want to loop through each of the dots related to each of the containers.
This means moving the init code down below all of the event code, so that we can call that event code from in there instead.

        // const containers = document.querySelectorAll(".slideshow-container");
        // var slideIndex = 1;
        // containers.forEach(function showFirstSlide(container) {
        //     showSlides(container, slideIndex);
        // });
...
        const dots = document.querySelectorAll(".dot");
        dots.forEach(function addDotHandlers(dot, index) {
            dot.addEventListener("click", function dotClickHandler(evt) {
                const dot = evt.target;
                addDotHandler(dot, index);
            });
        });
        const containers = document.querySelectorAll(".slideshow-container");
        var slideIndex = 1;
        containers.forEach(function showFirstSlide(container) {
            showSlides(container, slideIndex);
        });

First, we can add the slide handlers from within that containers foreach code:

        containers.forEach(function showFirstSlide(container) {
            addSlideHandlers(container);
            showSlides(container, slideIndex);
        });

We already have the addSlideHandlers code, we just need to put it into a function:

        function addSlideHandlers(container) {
            // const slideButtons = document.querySelectorAll(".prev, .next");
            const slideButtons = container.querySelectorAll(".prev, .next");
            slideButtons.forEach(function addSlideHandlers(slide) {
                slide.addEventListener("click", function slideClickHandler(evt) {
                    const el = evt.target;
                    addSlideHandler(el);
                });
            });
        }

And we can do the same with the dots. First use an addDotHandlers function:

        containers.forEach(function showFirstSlide(container) {
            addSlideHandlers(container);
            addDotHandlers(container);
            showSlides(container, slideIndex);
        });

Then put the already existing code inside an addDotHandlers function, that uses the container to find the dots to add.

        function addDotHandlers(container) {
            // const dots = document.querySelectorAll(".dot");
            const dots = getNextDots(container);
            dots.forEach(function addDotHandlers(dot, index) {
                dot.addEventListener("click", function dotClickHandler(evt) {
                    const dot = evt.target;
                    addDotHandler(dot, index);
                });
            });
        }

And the slide buttons and the dots now work on all of the slideshows.

TODO

The only things left to do are to finish off the TODO list, which will help to make sure that everything works properly.

  • Rename n to a more meaningful name
  • slideIndex shouldn’t be reassigned in the parameters of a function call
  • Let handlers find the container by walking up the DOM to the container
  • Separate slideIndex for each slideshow

I’ll work on that in the next post.

3 Likes

Here’s that TODO list:

  • Rename n to a more meaningful name
  • slideIndex shouldn’t be reassigned in the parameters of a function call
  • Let handlers find the container by walking up the DOM to the container
  • Separate slideIndex for each slideshow

Rename n

With the plusSlides and currentSlide functions, I can’t realistically call it index, because those start at 0. Instead I’ll just call it offset and num.

        function plusSlides(container, offset) {
            showSlides(container, slideIndex += offset);
        }
        function currentSlide(container, num) {
            showSlides(container, slideIndex = num);
        }

With the showSlides function, I can just call it num instad too.

        // function showSlides(container, n) {
        function showSlides(container, num) {
            var i;
            var slides = container.querySelectorAll(".mySlides");
            var dots = getNextDots(container);
            // if (n > slides.length) {slideIndex = 1}    
            if (num > slides.length) {slideIndex = 1}    
            // if (n < 1) {slideIndex = slides.length}
            if (num < 1) {slideIndex = slides.length}
            for (i = 0; i < slides.length; i++) {
                slides[i].style.display = "none";  
            }
            for (i = 0; i < dots.length; i++) {
                dots[i].className = dots[i].className.replace(" active", "");
            }
            slides[slideIndex-1].style.display = "block";  
            dots[slideIndex-1].className += " active";
        }

Fixing slideIndex to be properly an index

Doing this update helps me to see that the slideIndex variable isn’t really an index. Because it starts from 1, it’s more of a slideNum instead. For it to properly be an index it needs to start at 0.

I’ll start by renaming all of slideIndex to slideNum, then fix things up from there.

This is also a good time to get the reassign TODO item dealt with too.

        function plusSlides(container, num) {
            // showSlides(container, slideIndex += num);
            slideNum += num;
            showSlides(container, slideNum);
        }
        
        function currentSlide(container, num) {
            // showSlides(container, slideIndex = num);
            slideNum = num;
            showSlides(container, slideNum);
        }

The updated TODO list:

  • Rename n to a more meaningful name
  • lideIndex shouldn’t be reassigned in the parameters of a function call
  • Let handlers find the container by walking up the DOM to the container
  • Separate slideIndex for each slideshow

Renaming slideIndex to slideNum has a significant impact on the showSlides function:

        function showSlides(container, num) {
            var i;
            var slides = container.querySelectorAll(".mySlides");
            var dots = getNextDots(container);
            if (num > slides.length) {slideNum = 1}    
            if (num < 1) {slideNum = slides.length}
            for (i = 0; i < slides.length; i++) {
                slides[i].style.display = "none";  
            }
            for (i = 0; i < dots.length; i++) {
                dots[i].className = dots[i].className.replace(" active", "");
            }
            slides[slideNum-1].style.display = "block";  
            dots[slideNum-1].className += " active";
        }

And the last slideNum update is where we init the slideshow container:

        const containers = document.querySelectorAll(".slideshow-container");
        var slideIndex = 0;
        var slideNum = slideIndex + 1;
        containers.forEach(function showFirstSlide(container) {

Because we don’t have any tests for this code, I’ll do the conversion over from slideNum to slideIndex nice and slowly, to help make sure that no mistakes are made.

Assigning to slideIndex

The first part of the transition is that wherever slideNum is assigned, I want to add a slideIndex assignment there too.

        function plusSlides(container, num) {
            slideIndex += num;
            slideNum += num;
            showSlides(container, slideNum);
        }
        
        function currentSlide(container, num) {
            slideIndex = num - 1;
            slideNum = num;
            showSlides(container, slideNum);
        }
        ...
            // if (num > slides.length) {slideNum = 1}    
            if (num > slides.length) {
                slideIndex = 0;
                slideNum = 1;
            }
            // if (num < 1) {slideNum = slides.length}
            if (num < 1) {
                slideIndex = slides.length - 1;
                slideNum = slides.length;
            }
        ...
        const containers = document.querySelectorAll(".slideshow-container");
        var slideIndex = 0;
        var slideNum = 1;
        containers.forEach(function showFirstSlide(container) {

Change slideNum to be in reference to slideIndex instead

Then, replace slideNum with slideIndex references instead, where slideNum is one more than slideIndex.

        function plusSlides(container, num) {
            slideIndex += num;
            slideNum += num;
            // showSlides(container, slideNum);
            showSlides(container, slideIndex + 1);
        }
        
        function currentSlide(container, num) {
            slideIndex = num - 1;
            slideNum = num;
            showSlides(container, slideIndex + 1);
        }
        ...
            if (num > slides.length) {
                slideIndex = 1;
                // slideNum = 1;
                slideNum = slideIndex + 1;
            }
            if (num < 1) {
                slideIndex = slides.length - 1;
                // slideNum = slides.length
                slideNum = slideIndex + 1;
            }
        ...
            // slides[slideNum-1].style.display = "block";  
            slides[slideIndex].style.display = "block";  
            // dots[slideNum-1].className += " active";
            dots[slideIndex].className += " active";
        ...
        var slideIndex = 0;
        // var slideNum = 1;
        var slideNum = slideIndex + 1;
        ...
            // showSlides(container, slideNum);
            showSlides(container, slideIndex + 1);

Remove slideNum

As nothing uses slideNum in the code anymore, we can now remove the superfluous slideNum variables.

        function plusSlides(container, num) {
            slideIndex += num;
            // slideNum += num;
            showSlides(container, slideIndex + 1);
        }
        
        function currentSlide(container, num) {
            slideIndex = num - 1;
            // slideNum = num;
            showSlides(container, slideIndex + 1);
        }
        ...
            if (num > slides.length) {
                slideIndex = 0;
                // slideNum = 1;
            }
            if (num < 1) {
                slideIndex = slides.length - 1;
                // slideNum = slides.length;
            }
        ...
        var slideIndex = 0;
        // var slideNum = slideIndex + 1;

To properly convert things over, we also need to change num function parameters into index function parameters.

Use index values as well as num values

The plusSlides function is just an amount of change, so we should rename num to delta there instead.

        // function plusSlides(container, delta) {
        function plusSlides(container, delta) {
            slideIndex += delta;
            showSlides(container, slideIndex + 1);
            showSlides(container, slideIndex + 1, slideIndex);
        }

The other functions and their function calls can just have a separate index parameter added on to the function.

        function currentSlide(container, num, index) {
        function currentSlide(container, num) {
            // slideIndex = num - 1;
            slideIndex = num - 1;
            // showSlides(container, slideIndex + 1);
            showSlides(container, slideIndex + 1, slideIndex);
        }
        ...
        function showSlides(container, num, index) {
            ...
        }
        ...
        function addDotHandler(dot, index) {
            const container = getPrevContainer(dot);
            // currentSlide(container, index + 1);
            currentSlide(container, index + 1, index);
        }
        ...
            showSlides(container, slideIndex + 1, slideIndex);

Remove the num variable

And instead of using the num variable, which is one more than index, we can just use the index variable instead.

        function plusSlides(container, delta) {
            slideIndex += delta;
            // showSlides(container, slideIndex + 1, slideIndex);
            showSlides(container, slideIndex);
        }
        
        // function currentSlide(container, num, index) {
        function currentSlide(container, index) {
            // slideIndex = num - 1;
            slideIndex = index;
            // showSlides(container, slideIndex + 1, slideIndex);
            showSlides(container, slideIndex);
        }
        ...
        // function showSlides(container, num, index) {
        function showSlides(container, index) {
            ...
            // if (num > slides.length) {
            if (index > slides.length - 1) {
                slideIndex = 0;
            }
            // if (num < 1) {
            if (index < 0) {
                slideIndex = slides.length - 1;
            }
            ...
        }
        ...
        function addDotHandler(dot, index) {
            const container = getPrevContainer(dot);
            // currentSlide(container, index + 1, index);
            currentSlide(container, index);
        }
        ...
            // showSlides(container, slideIndex + 1, slideIndex);
            showSlides(container, slideIndex);

Next steps

There’s only one things remaining on the TODO list now:

  • Rename n to a more meaningful name
  • lideIndex shouldn’t be reassigned in the parameters of a function call
  • Let handlers find the container by walking up the DOM to the container
  • Separate slideIndex for each slideshow

We’ll get to that in the next post.

3 Likes

Currently with the slideIndex variable, that’s contained in the JavaScript code which doesn’t make it easily usable for multiple slideshows.

One option is to create separate instances of the slideshow code, each having different container and slideIndex properties, but a simpler solution that will work well here is to have slideIndex maintained on the HTML slideshow element itself, as a data attribute.

Setting and retrieving slideIndex value on container

We can have two separate functions called getIndex and setIndex, to help simplify the job for us.

        function getSlideIndex(container) {
            return Number(container.dataset.slideIndex);
        }
        function setSlideIndex(container, value) {
            container.dataset.slideIndex = value;
        }

Update slideIndex references

We can now easily update the code to use those functions.

        function plusSlides(container, delta) {
            // slideIndex += delta;
            const slideIndex = getSlideIndex(container) + delta;
            setSlideIndex(container, slideIndex);
            showSlides(container, slideIndex);
        }
        function currentSlide(container, index) {
            // slideIndex = index;
            setSlideIndex(container, index);
            // showSlides(container, slideIndex);
            showSlides(container, index);
        }
        ...
            if (index > slides.length - 1) {
                // slideIndex = 0;
                setSlideIndex(container, 0);
            }
            if (index < 0) {
                // slideIndex = slides.length - 1;
                setSlideIndex(container, slides.length - 1);
            }
            ...
            const slideIndex = getSlideIndex(container);
            slides[slideIndex].style.display = "block";  
            dots[slideIndex].className += " active";

        ...
        // var slideIndex = 0;
        containers.forEach(function showFirstSlide(container) {
            addSlideHandlers(container);
            addDotHandlers(container);
            setSlideIndex(container, 0);
            showSlides(container, 0);
        });

And the slides now properly move to each of their correct slide indexes, without interfering with each other.

Next steps

I will not be posting the full working code to get all of this done. Instead, I will use these existing posts to help guide you through all of the work that you must do to solve your problem.

[delete local test code] . . . [deleted]

Good luck.

3 Likes

Great set of posts @Paul_Wilkins :slight_smile:
I love the way you break everything down.
I might even attempt to implement that myself :).

1 Like

Thanks. Breaking things down is symptomatic of how I think about things. Big problems are most easily tackled by breaking it down in to smaller parts.

All of the above code is best understood as answering the following problem.

  • To get Slideshow1 + Slideshow2 working, first get Slideshow1 + Slideshow1 working.
2 Likes

Thanks so much for your help! I will take a closer look of this during the holiday. Merry Christmas!!

2 Likes

This topic was automatically closed 91 days after the last reply. New replies are no longer allowed.