2
\$\begingroup\$

I was just wanting a code review on this image slider I made. I don't have anyone else to give me their opinion on the code I write outside of work, so I'm asking here. okay so i had made an image slider and now i want to give an automatic option to it too. have any one any idea? feel free to tell me your answers.

thank you...

let slides = document.getElementsByClassName("slider__slide");
let navlinks = document.getElementsByClassName("slider__navlink");
let currentSlide = 0;

document.getElementById("nav-button--next").addEventListener("click", () => {
    changeSlide(currentSlide + 1)
});
document.getElementById("nav-button--prev").addEventListener("click", () => {
    changeSlide(currentSlide - 1)
});

function changeSlide(moveTo) {
    if (moveTo >= slides.length) {moveTo = 0;}
    if (moveTo < 0) {moveTo = slides.length - 1;}
    
    slides[currentSlide].classList.toggle("active");
    navlinks[currentSlide].classList.toggle("active");
    slides[moveTo].classList.toggle("active");
    navlinks[moveTo].classList.toggle("active");
    
    currentSlide = moveTo;
}

document.querySelectorAll('.slider__navlink').forEach((bullet, bulletIndex) => {
    bullet.addEventListener('click', () => {
        if (currentSlide !== bulletIndex) {
            changeSlide(bulletIndex);
        }
    })
})
body {
      min-height: 100vh;
}

.container {
    display: flex;
    align-items: center;
    justify-content: center;
}

.slider {
    display: block;
    position: relative;
    width: 100%;
    max-width: 900px;
    margin: 10px;
    background-color: white;
    overflow: hidden;
}

.slider__slides {
  width: 100%;
  padding-top: 66%;
}

.slider__slide {
    position: absolute;
    display: flex;
    align-items: center;
    justify-content: center;
    font-size: 50px;
    font-weight: bold;   
    top: 0;
    left: 0;
    width: 100%;
    height: 100%;
    transition: 3s;
    opacity: 0;
}

.active {
    opacity: 1;
}

.slider__slide img {
    width: 100%;
} 

.slider__nav-button {
    position: absolute;
    height: 70px;
    width: 70px;
    background-color: #333;
    opacity: .8;
    cursor: pointer;
}

#nav-button--prev {
    top: 50%;
    left: 0;
    transform: translateY(-50%);
}

#nav-button--next {
    top: 50%;
    right: 0;
    transform: translateY(-50%);
}

#nav-button--prev::after,
#nav-button--next::after {
    content: "";
    position: absolute;
    border: solid white;
    border-width: 0 4px 4px 0;
    display: inline-block;
    padding: 3px;
    width: 40%;
    height: 40%;
}

#nav-button--next::after{
    top: 50%;
    right: 50%;
    transform: translate(25%, -50%) rotate(-45deg);
}

#nav-button--prev::after {
    top: 50%;
    right: 50%;
    transform: translate(75%, -50%) rotate(135deg);
}

.slider__nav {
    position: absolute;
    bottom: 3%;
    left: 50%;
    transform: translateX(-50%);
    text-align: center;
}

.slider__navlink {
    display: inline-block;
    height: 20px;
    width: 20px;
    border-radius: 50%;
    border: 1px #fff solid;
    background-color: #333;
    opacity: 1;
    margin: 0 10px 0 10px;
    cursor: pointer;
}

.slider__navlink.active {
    background-color: #fff;
    border: 1px #333 solid;
}


@media screen and (max-width: 640px) {
  .slider__nav-button {
    height: 40px;
    width: 40px;
  }
  .slider__navlink {
    height: 12px;
    width: 12px;
  }
}
<!DOCTYPE html>
<html lang="en">
  <head>
    <meta charset="UTF-8">
    <meta name="viewport" content="width=device-width, initial-scale=1.0">
    <link rel="stylesheet" href="style.css">
  </head>
  <body>
    <h1 id="header"></h1>
<div class="container">
  <div class="slider">
    <div class="slider__slides ">
      <div class="slider__slide active">
         <img src="https://www.jowhareh.com/images/Jowhareh/galleries_8/poster_ded02296-0311-483b-a706-48b55ca65049.jpeg" alt="img1" />
      </div>
      <div class="slider__slide">
        <img src="https://www.jowhareh.com/images/Jowhareh/galleries_8/poster_c4c38355-e392-40e1-9cce-570b54780c8b.jpeg" alt="img2" />
      </div>
      <div class="slider__slide">
        <img src="https://www.jowhareh.com/images/Jowhareh/galleries_8/poster_29b5b00d-d38d-462d-a528-987516e77f64.jpeg" alt="img3" />
      </div>
      <div class="slider__slide">
        <img src="https://www.jowhareh.com/images/Jowhareh/galleries_8/poster_0acca158-34d3-4a9e-bb35-b598ccebbe08.jpeg" alt="img4" />
      </div>
    </div>
    <div id="nav-button--prev" class="slider__nav-button"></div>
    <div id="nav-button--next" class="slider__nav-button"></div>
    <div class="slider__nav">
      <div class="slider__navlink active"></div>
      <div class="slider__navlink"></div>
      <div class="slider__navlink"></div>
      <div class="slider__navlink"></div>
    </div>
  </div>
</div>
    <script src="script.js"></script>
  </body>
</html>

\$\endgroup\$
6
  • 1
    \$\begingroup\$ It looks good to me, but doesn't "auto" slide? I have to press something to get to the next image. \$\endgroup\$
    – KIKO Software
    Commented Jan 4 at 11:36
  • \$\begingroup\$ @KIKOSoftware Thank you for reviewing it. Yes, it is not automated yet. do you have any idea to help about it?I tried using setinterval method to make it auto, but my code messed up... \$\endgroup\$
    – hestia
    Commented Jan 5 at 11:44
  • 1
    \$\begingroup\$ Adding the automatic feature shouldn't be too difficult, and setinterval() is probably useful, but here we only review working code, hence the name: "Code Review". It would be more a question for the normal Stack Overflow, however I found a reasonable simple example which also detects when the mouse cursor is over the slider. You can copy parts of that code. \$\endgroup\$
    – KIKO Software
    Commented Jan 5 at 11:59
  • \$\begingroup\$ @KIKOSoftware Oh that's right and thanks for your example, it will help. \$\endgroup\$
    – hestia
    Commented Jan 5 at 12:24
  • \$\begingroup\$ Oh, the example uses JQuery, whereas you code in plain Javascript, so you cannot copy the code. Sorry for that. But not copying the code directly means more of the code is yours. 😊 \$\endgroup\$
    – KIKO Software
    Commented Jan 5 at 12:33

1 Answer 1

Reset to default
1
\$\begingroup\$

1 HTML

1.1 Indents

The indents within your body are incorrect. Always indent correctly to add readability.

1.2 Semantic tags

Use semantic tags always if suited. Semantic tags rais accessibility and also readability.

<div id="nav-button--prev" class="slider__nav-button"></div>
<div id="nav-button--next" class="slider__nav-button"></div>

In the above code lines, you have 2 buttons that require a user's input. A div as an unspecific container is not acceptable here. It is a control element that the user is supposed to click on. A button would have been more appropriate.

<div class="slider__nav">
  <div class="slider__navlink active"></div>
  <div class="slider__navlink"></div>
  <div class="slider__navlink"></div>
  <div class="slider__navlink"></div>
</div>

here you have multiple control elements which should be a list. A menu as the container containing buttons would have been correct here.

2 Javascript

2.1 const vs let

You declare only 3 variables from where only 1 is an actual variable.

let slides = document.getElementsByClassName("slider__slide");
let navlinks = document.getElementsByClassName("slider__navlink");
let currentSlide = 0;

In the code you posted, I would not expect the first 2 "variables" to change. As such they are technical constants.
You should use a const wherever suited and mark them by writing them all capitals so you can spot differences between variables and constants.

2.2 getElementsByClassName vs querySelectorAll

getElementsByClassName will return an HTML Collection Object while querySelectorAll will return a *Node List. Both are an array-like list with only a few differences. getElementsByClassName will be slightly faster within nanoseconds. querySelectorAll can select elements with the same rules as CSS and not only classes but also allows the usage of forEach as an iteration method.

/* JS:2  */   let navlinks = document.getElementsByClassName("slider__navlink");
/* JS:24 */   document.querySelectorAll('.slider__navlink')

If you would have used document.querySelectorAll it JS-line 2 then you could have used that constant later on instead of writing the whole document selector again.

2.3 Modulus Operator

You can use the modulus (%) to get the remainder of a division. This can help you in certain cases to shorten calculations such as infinite iterations. You have used:

if (moveTo >= slides.length) {moveTo = 0;}
if (moveTo < 0) {moveTo = slides.length - 1;}

You have 2 "slow" if statements while a simple modulus calculation would be shorter and faster such as:

moveTo = (moveTo + slides.length) % slides.length;


let moveTo= 0;
const slides = [1, 2, 3, 4];

function changeSlide() {
  moveTo = (moveTo + slides.length) % slides.length;
  console.log(moveTo);
}


/* just to be demostratable */
document.querySelectorAll('button').forEach(btn => 
  btn.addEventListener('click', function() {
    console.clear();
    switch (this.id) {
      case 'plus':
        moveTo++;
        break;
      case 'minus':
        moveTo--;
        break;
    }
    changeSlide();
  })                                           
)
<button id="plus">+</button>
<button id="minus">-</button>

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.