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>1 Answer 1
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>
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\$