0

I have copied and modified a W3schools code, and now it just bugs out. I have set an interval of 2000 (2 seconds) and it slides the images how it should, however when I click on the next or previous buttons, it just simply slides too fast, sometimes it slides 3 times under one second, or 1-2 slides without clicking on it. (Looks like it slides fast (after clicking) on some parts).

And after I click on the buttons several times it just slides insanely fast until I refresh the page, which remains working how it should as long as I don't click on any prev or next button.

Here is the HTML code

    <div class="main-slideShow-div">
    <div class="slideshow-container">
        <div class="my-slides">
                <img class="pic" src="images/animals/dog1.jpg">
        </div>

        <div class="my-slides">
                <img class="pic" src="images/animals/dog2.jpg">
        </div>

        <div class="my-slides">
                <img class="pic" src="images/animals/dog3.jpg">
        </div>
    
        <img class="prev" onclick="plusSlides(-1)" src="images/rightArrow.png" alt="picture">
        <img class="next" onclick="plusSlides(1)" src="images/leftArrow.png" alt="picture">
        
        <div class="slider-div" style="text-align:center">
            <div class="line" onclick="slideIndicator(1)"></div>
            <div class="line" onclick="slideIndicator(2)"></div>
            <div class="line" onclick="slideIndicator(3)"></div>
        </div>
    </div>
    </div>
    <script src="javascript1.js"></script>

And here is the JavaScript code

        var slideIndex = 1;
        showSlides(slideIndex);

        // Next/previous controls
        function plusSlides(n) 
        {
            showSlides(slideIndex += n);
        }

        // Thumbnail image controls
        function slideIndicator(n) 
        {
            showSlides(slideIndex = n);
        }

        function showSlides(n) 
        {
            var i;
            var slides = document.getElementsByClassName("my-slides");
            var line = document.getElementsByClassName("line");
            slideIndex++;
            if (slideIndex > 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 < line.length; i++) 
            {
                line[i].className = line[i].className.replace(" active", "");
            }
            slides[slideIndex-1].style.display = "block";
            line[slideIndex-1].className += " active";
            setTimeout(showSlides, 2000);
        }

I'm aware that it increments the slider too many times, but can't figure out where.

Sorry for asking about a trivial code, I guess it should be understandable for me that what the problem is, but i'm just started using javascript.

And just one more question: what is that showSlides at the beginning? If it is a global variable than how can you call it as a function later on?

Any help would be appreciated! :D

Idontknowhowtocode
  • 61
  • 6
  • `showSlides` looks like the likely culprit here (actually, it's only a part). The function names hints at its purpose, yet when we look inside it, we see something a little alarming on the 4th line: `slideIndex++;` - Hang on a minute - I thought we were just displaying em here. Crap, we're changing stuff too. Hopefully, it wont change slideIndex in a bunch of other places too. :looks: Oooh. Have functions for changing the current slide and another for displaying them. Your car key is your car key - it's not a screwdriver or an ear scratcher too. Let your code perform its own tasks. – enhzflep Mar 20 '21 at 20:51
  • I am trying to write this on my own, but I got a little problem that I am trying to solve for like 2 hours now. Here is the code I wrote https://jsfiddle.net/7b4yuc9o/3/ What I'm trying to do is if you click on that blue div it sets the background of the yellow div to green. I tried other more complicated things first, but I came back to this, since I cant even get the simplest thing to work. What am I missing? – Idontknowhowtocode Mar 20 '21 at 23:10
  • Ahhhhhh! That's an easy fix. From now on, use the Mozilla MDN for all your javascript reference material. You've used `.getElementsByClassName` - notice it's elements - plural. That function returns *an array*, yet you've treated it like it returns a single element. When you run the fiddle, a little exclamation mark (!) turns red in the bottom right - this alerts you to the fact there are errors. It even tells you what they are - sorta, kinda. "Uncaught TypeError: Cannot set property 'backgroundColor' of undefined" - You need to access element 0, so add `[0]` right before `.style` – enhzflep Mar 20 '21 at 23:23
  • Here's an updated version of that fiddle. I just added the array index operator `[ ]`and an index `(0)` The entire body of the `next` function is now: `document.getElementsByClassName("div1")[0].style.backgroundColor="#0f0";` https://jsfiddle.net/97kcgv12/ – enhzflep Mar 20 '21 at 23:25
  • Maaaan, ty so much, I saw that it is plural (Elements) but I thought that it means nothing, I didnt know that I should treat it as an array. :D – Idontknowhowtocode Mar 20 '21 at 23:45
  • :) You're most welcome. Also, wait till you try the javascript debugger if you haven't already. It's _amazing_ - you can check all sorts of thing while it's running or paused. Trying to work without it, and you may as well be a blind painter - since you've now got so little idea of the environment that surrounds you. Ctrl-Shift-I or, you can find it in the Developer Tools. – enhzflep Mar 20 '21 at 23:55
  • And what could be the problem with that? https://jsfiddle.net/y5m8toqz/4/ I mean, it works in fiddle, but it doesnt when I run it "natively" – Idontknowhowtocode Mar 21 '21 at 00:02
  • Let us [continue this discussion in chat](https://chat.stackoverflow.com/rooms/230173/discussion-between-enhzflep-and-idontknowhowtocode). – enhzflep Mar 21 '21 at 00:57

2 Answers2

1

<div class="main-slideShow-div">
  <div class="slideshow-container">
    <div class="my-slides">
      <span>111</span>
    </div>

    <div class="my-slides">
      <span>112</span>
    </div>

    <div class="my-slides">
      <span>113</span>
    </div>

    <img class="prev" onclick="plusSlides()" alt="<" />
    <img class="next" onclick="minusSlides()" alt=">" />

    <div class="slider-div" style="text-align: center;">
      <div class="line" onclick="slideIndicator(1)">1</div>
      <div class="line" onclick="slideIndicator(2)">2</div>
      <div class="line" onclick="slideIndicator(3)">3</div>
    </div>
  </div>
</div>
<script>
  var slideIndex = 1;
  showSlides(slideIndex);

  // Next/previous controls
  function plusSlides() {
    showSlides(slideIndex++);
  }
  function minusSlides() {
    showSlides(slideIndex--);
  }

  // Thumbnail image controls
  function slideIndicator(n) {
    showSlides((slideIndex = n));
  }

  function showSlides(n) {
    var i;
    var slides = document.getElementsByClassName("my-slides");
    var line = document.getElementsByClassName("line");

    if (slideIndex >= slides.length) {
      slideIndex = 1;
    } else if (slideIndex < slides.length) {
      slideIndex++;
    }
    if (n < 1) {
      slideIndex = slides.length;
    }
    for (i = 0; i < slides.length; i++) {
      slides[i].style.display = "none";
    }
    for (i = 0; i < line.length; i++) {
      line[i].className = line[i].className.replace(" active", "");
    }
    slides[slideIndex - 1].style.display = "block";
    line[slideIndex - 1].className += " active";
    setTimeout(showSlides, 2000);
  }
</script>

First of all, this is a very bad piece of code. Let me try to list some of the reasons I can find.

  1. Calling showSlides too many times, which is causing this to get ever faster when you click on any arrow, as the function keeps getting called again and again making is faster as it is running multiple times.
  2. It will fail many grey test cases as no proper boundary conditions.
  3. It is calling prev/next but not reducing/increasing the index, instead it is calling it with a value.

I have tried to fix it by writing an else statement which will check the siteIndex before incrementing. Also, tried to increment/decrement index instead of calling showSlides with an integer.

Note: The time problem will never get fixed as long as we keep calling the showSlides function everytime we click a button. I would suggest you try to fix this on your own, by calling showSlides just once and instead of taking n as argument it uses the siteIndex automatically and just changing siteIndex on clicks. For the question about showSlides.

It is just a function, there is no global/local in JS. Although, if you want to explore that you can read about closures in JS.

We are calling it in the start to initialize the sliding, and hide the other divs. You can remove the initial call and check for yourself.

Anmol Bhardwaj
  • 636
  • 6
  • 18
  • Okay, I get it now, so how should I write that? Not asking for exact code, I want to figure it out on my own, but what would be the way to go? Can I put the ```plusSlides``` and ```minusSlides``` inside the function with a ```getElement....``` something which does something when you click on it, so you doesn't need to call the function? – Idontknowhowtocode Mar 20 '21 at 21:03
  • yeah, don't take slideIndex as an argument, instead , try to change the showSlides function to rely on the current value of slideIndex. This way , you would be only calling the function once and it will be relying on the value of slideIndex, and you can change the value onClick of different buttons. – Anmol Bhardwaj Mar 21 '21 at 10:46
1

So I had a look on the w3schools slideshows and it seems that you modyfied it.

According to your question you're incrementing slideIndex in showSlides function. Can you tell me, why you would like to increment the slideIndex ? So I believe that's the place where it is incrementing...

Also in the end of the showSlides function you've got setTimeout which should delay the function it self and it should look like this setTimeout(showSlides(), 2000) not setTimeout(showSlides, 2000) if you're calling function use () parentheses and also you can refer to this question

About your last question

what is that showSlides at the beginning?

It's not a global variable

Global variables are variables that are accessible for whole document and starting by declaring with keyword var, let, const.

This is just a simple function call for showSlides function

catJam
  • 224
  • 1
  • 10