JS Simple Image Slider

Question

I'm currently learning some javascript and I've been getting up to scratch with ES6. I have learned the basics and continued to build things that I've wanted to build in Javascript for a long time.

I am looking for someone to review the code, and to hopefully aid me in becoming a better developer in learning from mistakes.

So the image slider is working from an array which is a div with 4 hidden img elements. The is an img element which is being created within the div #image-display id. The JS then swaps the src attribute based off the number within the counter and the key from the array.

The working copy is on codepen: https://codepen.io/syncoed/pen/WmpEQJ

HTML

    <div id="image-display" class="mx-auto">
        <span id="left">&#10096;</span>
        <span id="right">&#10097;</span>
    </div>

    <div id="img-container">
        <img id="img-size" src="../image-slider/img/image1.jpg" hidden/>
        <img id="img-size" src="../image-slider/img/image2.jpg" hidden/>
        <img id="img-size" src="../image-slider/img/image3.jpg" hidden/>
        <img id="img-size" src="../image-slider/img/image4.jpg" hidden/>
    </div>

</div>

CSS

 #image-display{
 width:500px;
 height: 100%;
 border: 3px solid orange;
 z-index: -1;
 padding:3;
 margin:0;
}

#img-size{
 height:100%;
 width:100%;
 z-index: -1;
 margin:0;
 position: relative;
}

/* left arrow */
#left{
 z-index: 0;
 position: fixed;
 left:21.5%;
 top:10%;
 cursor: pointer;
 color:#ffffff;
 padding:0;
}


/* right arrow */
 #right{
 z-index: 0;
 position: fixed;
 right:53.5%;
 top: 10%;
 cursor: pointer;
 color:#ffffff;
 padding:0;
}


/*Desktop Media Queries*/

@media(min-width: 1573px) {

#right{
    right:35%;
    top: 12%;
}

#left{
    left:35.5%;
    top:12%;
 }
}

@media(min-width: 1613px) {

#right{
    right:35.5%;
    top: 11%;
}

#left{
    left:35.5%;
    top:11%;
 }
}

@media(min-width: 1140px) {

#right{
    right:37.5%;
    top: 11%;
}

#left{
    left:37.5%;
    top:11%;
 }
}

Javascript

 const imgDisplay = document.querySelector('#image-display');
 const allImages = document.querySelector('#img-container');

 //gets all images
 let all = allImages.querySelectorAll('img');

var imageContainer = document.querySelector('.selector');
let counter = 0;
var newImg = document.createElement('img');
newImg.setAttribute('id', 'img-size');

newImg.setAttribute('src', all[0].src);
imgDisplay.appendChild(newImg);

let imageSelected = all[counter];



imgDisplay.addEventListener('click', (e) => {

  if (counter == all.length - 1 && e.target.id == "right") {

    newImg.setAttribute('src', all[0].src);
    counter = 0;

 } else if (counter == 0 && e.target.id == 'left') {

    counter = all.length - 1;
    newImg.setAttribute('src', all[counter].src);

 } else {

    if (e.target.tagName == 'SPAN' && e.target.id == 'right') {
        counter++;

        newImg.setAttribute('src', all[counter].src);
    }

    if (e.target.tagName == 'SPAN' && e.target.id == 'left') {

        counter--;
        newImg.setAttribute('src', all[counter].src);

    }
 }



});

Answer 1

Explanation:

If you're going to take the source of an image and set it to another image, why even bother having those images as HTML? It would be much simpler to simply store the image urls in an array like so:

const imageUrls = [
  "https://i.ytimg.com/vi/E6CYI3Xb1tQ/maxresdefault.jpg",
  "https://upload.wikimedia.org/wikipedia/commons/9/9b/Dragon_Ball_Z_Logo.png",
  "https://encrypted-tbn0.gstatic.com/images?q=tbn:ANd9GcQDWVk-v3DR5j0pHabogu6i3n1KLhqJWNd-Z54qevISiCoHKF88",
  "https://media.comicbook.com/2017/02/vegeta-dragon-ball-z-235088-1280x0.png"
];

Secondly, you're using a counter variable and an imageSelected variable... but their end goal is the same thing. Stick with the counter.

You're spending a lot of time writing conditions, why not use % (modulo) to determine which is the next image.

When the right button is clicked simply do:

counter = (counter + 1) % imageUrls.length

This way, once you get to the last image (counter = 3) and wish to go to the next one, then (counter + 1) = (3+1) = (4). So the new position is 4 however the imageUrl length is 4.. and 4 % 4 = 0. So, it's an easy solution to reset the counter to 0 when it gets to the end of the image url list.

However, this doesn't work when you wish to go left. Because module doesn't work on negative numbers. To solve this do the following.

counter = (counter + -1 + imageUrls.length) % imageUrls.length

Now, to figure out if it's 1 or -1 and to avoid setting a condition for it, we're going to store those values in the data attributes in the left and right HTML elements like so:

<span data-direction="1" id="left">&#10096;</span>
<span data-direction="-1" id="right">&#10097;</span>

Then simply to access them in the event callback:

Number(this.dataset.direction) //returns a string so reason why use Number to convert it.

Now, all there is left to do is to complete the formula.

counter = (counter + Number(this.dataset.direction) + imageUrls.length) % imageUrls.length;

Avoid setting the onclick function to a container and actually set it to the elements that need to be clicked.

imgDisplay.querySelectorAll("#left, #right").forEach(element=>{
  //first time this is called element = left
  //second time this is called element = second
  //for each element add an event listener.
});

Solution:

const imageUrls = [
  "https://i.ytimg.com/vi/E6CYI3Xb1tQ/maxresdefault.jpg",
  "https://upload.wikimedia.org/wikipedia/commons/9/9b/Dragon_Ball_Z_Logo.png",
  "https://encrypted-tbn0.gstatic.com/images?q=tbn:ANd9GcQDWVk-v3DR5j0pHabogu6i3n1KLhqJWNd-Z54qevISiCoHKF88",
  "https://media.comicbook.com/2017/02/vegeta-dragon-ball-z-235088-1280x0.png"
];

const imgDisplay = document.getElementById('image-display');


let counter = 0;
const img = new Image();
img.src = imageUrls[counter];
imgDisplay.appendChild(img);

imgDisplay.querySelectorAll("#left, #right").forEach(element=>{
  element.addEventListener('click', function(e){
      counter = (counter + Number(this.dataset.direction) + imageUrls.length) % imageUrls.length;
      img.src = imageUrls[counter];
  });
});

Working Example:

const imageUrls = [
  "https://i.ytimg.com/vi/E6CYI3Xb1tQ/maxresdefault.jpg",
  "https://upload.wikimedia.org/wikipedia/commons/9/9b/Dragon_Ball_Z_Logo.png",
  "https://encrypted-tbn0.gstatic.com/images?q=tbn:ANd9GcQDWVk-v3DR5j0pHabogu6i3n1KLhqJWNd-Z54qevISiCoHKF88",
  "https://media.comicbook.com/2017/02/vegeta-dragon-ball-z-235088-1280x0.png"
];

const imgDisplay = document.getElementById('image-display');


let counter = 0;
const img = new Image();
img.src = imageUrls[counter];
imgDisplay.appendChild(img);

imgDisplay.querySelectorAll("#left, #right").forEach(element=>{
  element.addEventListener('click', function(e){
      counter = (counter + Number(this.dataset.direction) + imageUrls.length) % imageUrls.length;
      img.src = imageUrls[counter];
  });
});

#image-display {
  width: 500px;
  height: 100%;
  border: 3px solid orange;
  z-index: -1;
  padding: 3;
  margin: 0;
}

#img-size {
  height: 100%;
  width: 100%;
  z-index: -1;
  margin: 0;
  position: relative;
}


/* left arrow */

#left {
  z-index: 0;
  position: fixed;
  left: 21.5%;
  top: 10%;
  cursor: pointer;
  color: #ffffff;
  padding: 0;
}


/* right arrow */

#right {
  z-index: 0;
  position: fixed;
  right: 53.5%;
  top: 10%;
  cursor: pointer;
  color: #ffffff;
  padding: 0;
}


/*Desktop Media Queries*/

@media(min-width: 1573px) {
  #right {
    right: 35%;
    top: 12%;
  }
  #left {
    left: 35.5%;
    top: 12%;
  }
}

@media(min-width: 1613px) {
  #right {
    right: 35.5%;
    top: 11%;
  }
  #left {
    left: 35.5%;
    top: 11%;
  }
}

@media(min-width: 1140px) {
  #right {
    right: 37.5%;
    top: 11%;
  }
  #left {
    left: 37.5%;
    top: 11%;
  }
}

<html>

<head>
  <meta charset="utf-8">
  <meta http-equiv="X-UA-Compatible" content="IE=edge">
  <title></title>
  <meta name="viewport" content="width=device-width, initial-scale=1">

  <link rel="stylesheet" href="https://stackpath.bootstrapcdn.com/bootstrap/4.3.1/css/bootstrap.min.css" integrity="sha384-ggOyR0iXCbMQv3Xipma34MD+dH/1fQ784/j6cY/iJTQUOhcWr7x9JvoRxT2MZw1T" crossorigin="anonymous">

</head>

<body>
  <div class="container">

    <div id="image-display" class="mx-auto">
      <!-- <img id="img-size" class="selector" /> -->
      <span data-direction="1" id="left">&#10096;</span>
      <span data-direction="-1" id="right">&#10097;</span>
    </div>
  </div>
</body>

</html>