How to get this button to fire the appropriate event

Question

I am trying to get my slider interactive on a test i'm working on and I just can't figure it out. I have some code I have utilised from W3 but I'm trying to add a couple of events for the buttons so it is all on it's own javascript file instead of embedded in the HTML.

Here is the HTML:

<!DOCTYPE html>
<html>
<title>W3.CSS</title>
<meta name="viewport" content="width=device-width, initial-scale=1">
<link rel="stylesheet" href="https://www.w3schools.com/w3css/4/w3.css">
<script src="main.js" defer></script>

<body>

<h2 class="w3-center">Manual Slideshow</h2>

<div class="w3-content w3-display-container">
  <img class="mySlides" src="./image_1.jpg" style="width:100%">
  <img class="mySlides" src="./image_2.jpg" style="width:100%">
  <img class="mySlides" src="./image_3.jpg" style="width:100%">

  <button id="clicker" class="w3-button w3-black w3-display-left">&#10094;</button>
  <button id="clicker" class="w3-button w3-black w3-display-right">&#10095;</button>
</div>

</body>
</html>

Here is the Javascript:

let leftSlideButton = document.getElementById("clicker");
let rightSlideButton = document.getElementById("clicker");

var slideIndex = 1;
showDivs(slideIndex);

function plusDivs(n) {
  showDivs(slideIndex += n);
}

function showDivs(n) {
  var i;
  var x = document.getElementsByClassName("mySlides");
  if (n > x.length) {slideIndex = 1}
  if (n < 1) {slideIndex = x.length}
  for (i = 0; i < x.length; i++) {
    x[i].style.display = "none";  
  }
  x[slideIndex-1].style.display = "block";  
}

let slideForward = plusDivs(1);
let slideBack = plusDivs(-1);

leftSlideButton.onclick = slideBack;
rightSlideButton.onclick = slideForward;

The slider worked when the script was embedded in the hmtl but when I tried to write in some functionality for the buttons in their own javascript file it just stops working and I'm pretty stumped now. Iv'e added them to their own variable and grabbed it's html selector so I can in turn add an event handler and link it to the function but it just doesn't work. Any help would be appreciated.

Also, this is my first post so apologies if I haven't formatted it correctly.

TIA, Neil.

Denis O. · Accepted Answer · 2019-09-28T01:45:30.893

Most likely the problem caused by 2 x id="clicker". Using unique ids for the buttons should solve to problem.

Next, since slideIndex declared globally it doesn't require plusDivs middle-ware.

I would also suggest to use clearer functions and variables names for the sake of better readability. Check these: slideDivs vs showDivs, step vs n, slides vs x, etc. It maybe doesn't sound like a big deal on the given example, but it is a good habit and will save you a lot of time when working on bigger projects.

There are also some minor optimizations possible, such as changing visibility in one loop and caching slides outside of sliding function to avoid selecting it again on every click. Check the snipped for suggested rewritten code.

var visibleSlideIndex = 0;
var slides = document.getElementsByClassName("mySlides");

function slideDivs(step) {
    visibleSlideIndex = (visibleSlideIndex + step + slides.length) % slides.length;
    for (let index = 0; index < slides.length; index++) {
        slides[index].style.display = visibleSlideIndex === index ? "block" : "none";
    }
}

document.getElementById("slideLeftBtn").onclick = () => slideDivs(1);
document.getElementById("slideRightBtn").onclick = () => slideDivs(-1);
slideDivs(0);

<!DOCTYPE html>
<html>
<title>W3.CSS</title>
<meta name="viewport" content="width=device-width, initial-scale=1">
<link rel="stylesheet" href="https://www.w3schools.com/w3css/4/w3.css">
<script src="main.js" defer></script>

<body>

<h2 class="w3-center">Manual Slideshow</h2>

<div class="w3-content w3-display-container">
 <img class="mySlides" src="./image_1.jpg" style="background-color: red;">
 <img class="mySlides" src="./image_2.jpg" style="background-color: green;">
 <img class="mySlides" src="./image_3.jpg" style="background-color: blue;">
  
 <button id="slideLeftBtn" class="w3-button w3-black w3-display-left">&#10094;</button>
 <button id="slideRightBtn" class="w3-button w3-black w3-display-right">&#10095;</button>
</div>

</body>
</html>

I was using some code from W3 and adding to it myself but this does make it clearer to understand. — neilvix, Sep 28 '19 at 09:16
**Always code as if the guy who ends up maintaining your code will be a violent psychopath who knows where you live.** (c) Martin Golding It is always a good habit to a) rewrite and understand code instead of copy-paste, b) use a clear functions and variables naming to avoid spending time __when__ you (or someone else) need to get back to it a while after. Check this post for some good practices, it will save you a lot of time in future: https://devinduct.com/blogpost/22/javascript-clean-code-best-practices — Denis O., Sep 28 '19 at 12:08

score 0 · Answer 2 · answered Sep 27 '19 at 23:07

You have multiple issues :

First, the button selection. You are trying to get the 2 buttons with the same id, you have to change that.

Then, those 2 lines :

let slideForward = plusDivs(1);
let slideBack = plusDivs(-1);

plusDivs(1) and plusDivs(-1) are calling the function and returning the value. To fix it, you can create an anonymous function (or an arrow function) to call plusDivs

Here is a fixed version (I changed the width of the images to 25% so it would be easier to see in the snippet):

let leftSlideButton = document.getElementById("clickerLeft");
let rightSlideButton = document.getElementById("clickerRight");

var slideIndex = 1;
showDivs(slideIndex);

function plusDivs(n) {
  showDivs(slideIndex += n);
}

function showDivs(n) {
  var i;
  var x = document.getElementsByClassName("mySlides");
  if (n > x.length) {slideIndex = 1}
  if (n < 1) {slideIndex = x.length}
  for (i = 0; i < x.length; i++) {
    x[i].style.display = "none";  
  }
  x[slideIndex-1].style.display = "block";  
}


leftSlideButton.onclick = () => plusDivs(1);
rightSlideButton.onclick = () => plusDivs(-1);

<h2 class="w3-center">Manual Slideshow</h2>

<div class="w3-content w3-display-container">
  <img class="mySlides" src="https://via.placeholder.com/150" style="width:25%">
  <img class="mySlides" src="https://via.placeholder.com/200" style="width:25%">
  <img class="mySlides" src="https://via.placeholder.com/250" style="width:25%">

  <button id="clickerLeft" class="w3-button w3-black w3-display-left">&#10094;</button>
  <button id="clickerRight" class="w3-button w3-black w3-display-right">&#10095;</button>
</div>

This worked for me. I hadn't even realized that I was using the same 'Id' for both buttons. I appreciate the thorough explanation. — neilvix, Sep 28 '19 at 09:12

score 0 · Answer 3 · answered Sep 27 '19 at 23:11

You can't have two identical ids, so I gave the buttons separate ids, then I attached an event listener to each one with addEventListener. Inside each listener I added the plusDivs() function directly in.

let leftSlideButton = document.getElementById("clicker-left");
let rightSlideButton = document.getElementById("clicker-right");

var slideIndex = 1;
showDivs(slideIndex);

function plusDivs(n) {
  showDivs(slideIndex += n);
}

function showDivs(n) {
  var i;
  var x = document.getElementsByClassName("mySlides");
  if (n > x.length) {slideIndex = 1}
  if (n < 1) {slideIndex = x.length}
  for (i = 0; i < x.length; i++) {
    x[i].style.display = "none";  
  }
  x[slideIndex-1].style.display = "block";  
}

leftSlideButton.addEventListener("click",  () => {plusDivs(1)});
rightSlideButton.addEventListener("click", () => {plusDivs(-1)});

.w3-content > img{
   max-height: 200px;
   object-fit: contain;
   background-color: lightblue;
}

<h2 class="w3-center">Manual Slideshow</h2>

<div class="w3-content w3-display-container">
  <img class="mySlides" src="http://placekitten.com/100/200" style="width:100%">
  <img class="mySlides" src="http://placekitten.com/200/200" style="width:100%">
  <img class="mySlides" src="http://placekitten.com/150/200" style="width:100%">

  <button id="clicker-left" class="w3-button w3-black w3-display-left">&#10094;</button>
  <button id="clicker-right" class="w3-button w3-black w3-display-right">&#10095;</button>
</div>

I hadn't even noticed about the same 'Id' being used twice. I considered using an event listener but I couldn't figure out how to write it in code to make it work. — neilvix, Sep 28 '19 at 09:19
@neilvix the `element.onclick = () => {}` format is an event listener, too. Best practices recommended that you use `addEventListener()` however: https://stackoverflow.com/questions/6348494/addeventlistener-vs-onclick — symlink, Sep 28 '19 at 18:00

How to get this button to fire the appropriate event

3 Answers3