I am new to JavaScript and jQuery, and I would like to know what can be improved of the code in this slideshow.
(function ($) {
let pointer = 0;
let slides = document.getElementsByClassName('mySlide');
for (let i = 0; i < slides.length; i++) {
slides[i].style.left = 100 * i + "%";
$('#dots').append('<span class="dot"></span>');
}
function changeSlide() {
$('#slider').css('left', -100 * pointer + '%');
changeDot();
pointer++;
if (pointer > (slides.length - 1)) pointer = 0;
setTimeout(changeSlide, 3000);
}
$(document).ready(function () {
changeSlide();
});
})(jQuery);
As can be observed in the code, I'm changing the left property of the <div>
inside another <div>
with overflow: hidden
where the images are set to position: absolute
.
1 Answer 1
Cache DOM lookups
When iterating over the elements with class name mySlide in the for
loop, there is a look-up for the dots container (i.e. $('#dots')
). Bearing in mind that the example only contains four slide elements, that could lead to excess DOM queries. It is good to store references like that and then utilize the reference:
const dotsContainer = $('#dots');
//later: (e.g. in for loop)
dotsContainer.append('<span class="dot"></span>');
Consistency
Why mix native JavaScript (e.g. getting the elements with class name mySlide: document.getElementsByClassName('mySlide');
) and then use jQuery later on (e.g. getting elements with class name dot: $('.dot')
?
That first block to loop over the elements with class name mySlide could be re-written in jQuery style:
const dotsContainer = $('#dots'); //cache the DOM lookup for later
const slides = $('.mySlide');//document.getElementsByClassName('mySlide');
slides.each(function(i, element) { //for (let i = 0; i < slides.length; i++) {
$(element).css({left: 100 * i + "%"});//slides[i].style.left = 100 * i + "%";
dotsContainer.append('<span class="dot"></span>');
});
Closure simplification
Some of the closures can be simplified - for instance:
$('.dot').each(function () { $(this).removeClass('is-active'); });
can be simplified to:
$('.dot').removeClass('is-active');
And the same is true for the DOM-ready callback:
$(document).ready(function () { changeSlide(); });
Can be simplified to:
$(document).ready(changeSlide);
jQuery DOM-ready syntax:
As of jQuery 3.0, only $(handler)
is recommended; the other syntaxes still work but are deprecated.1
So that ready callback registration can be simplified to:
$(changeSlide);
Many of these tips are explained in more detail in this article from a couple years avo. It does start off by making one question how much jquery is really needed these days- a valid question. For more on that topic, check out you might not need jquery.
(function ($) {
let pointer = 0;
//store instead of looking up each time
const dotsContainer = $('#dots');
let slides = $('.mySlide');//document.getElementsByClassName('mySlide');
slides.each(function(i, element) { //for (let i = 0; i < slides.length; i++) {
$(element).css({left: 100 * i + "%"});//slides[i].style.left = 100 * i + "%";
dotsContainer.append('<span class="dot"></span>');
});
$('.dot').each(function (i) {
$(this).on('click', function () {
$('#slider').css('left', -100 * i + '%');
pointer = i;
changeDot();
});
});
function changeDot() {
$('.dot').removeClass('is-active');
$('.dot').eq(pointer).addClass('is-active');
}
function changeSlide() {
$('#slider').css('left', -100 * pointer + '%');
changeDot();
pointer++;
if (pointer > (slides.length - 1)) pointer = 0;
setTimeout(changeSlide, 3000);
}
$(changeSlide);
})(jQuery);
body {
background: #28262b;
color: white;
font-family: 'Roboto', serif-sans;
}
#slider-wrap {
width: 700px;
height: 500px;
background: #39373a;
border: 1px solid #17151c;
text-align: center;
margin: 0 auto;
overflow: hidden;
}
img {
width: 100%;
height: 100%;
}
#slider {
height: 500px;
left: 0;
position: relative;
transition: all 300ms ease-in-out;
}
.mySlide {
position: absolute;
text-align: center;
z-index: 100;
top: 0;
left: 0;
height: 500px;
}
.photo {
position: relative;
width: 700px;
height: 100%;
}
.description {
position: absolute;
bottom: 0;
left: 0;
text-align: center;
padding: 20px 0;
width: 100%;
height: 100px;
background: rgba(0,0,0,.7);
}
#dots {
display: flex;
align-items: center;
justify-content: center;
margin: 20px;
}
.dot {
display: block;
border: 2px solid white;
border-radius: 50%;
margin: 0 10px;
width: 15px;
height: 15px;
}
.dot:hover {
cursor: pointer;
background: #616161;
}
.is-active {
background: white;
}
.dot.is-active:hover {
background: pink;
border-color: pink;
}
<script src="https://ajax.googleapis.com/ajax/libs/jquery/3.2.1/jquery.min.js"></script>
<body>
<div id="slider-wrap">
<div id="slider">
<div class="mySlide">
<div class="photo">
<img src='https://www.fillmurray.com/300/300'/>
</div>
<span class="description">test1</span>
</div>
<div class="mySlide">
<div class="photo">
<img src='https://www.fillmurray.com/301/300'/>
</div>
<span class="description">test2</span>
</div>
<div class="mySlide">
<div class="photo">
<img src='https://www.fillmurray.com/302/300'/>
</div>
<span class="description">test3</span>
</div>
<div class="mySlide">
<div class="photo">
<img src='https://www.fillmurray.com/303/300'/>
</div>
<span class="description">test4</span>
</div>
</div>
</div>
<div id="dots"></div>
</body>
1http://api.jquery.com/ready/
-
\$\begingroup\$ thank you so much for this long hardly helpful comment! wow \$\endgroup\$Davkk– Davkk2017年12月02日 09:47:28 +00:00Commented Dec 2, 2017 at 9:47
-
\$\begingroup\$ You're welcome, but what do you mean by hardly? \$\endgroup\$2017年12月03日 14:40:15 +00:00Commented Dec 3, 2017 at 14:40
-
\$\begingroup\$ @SᴀᴍOnᴇᴌᴀ English by foreigners \$\endgroup\$Joop Eggen– Joop Eggen2021年10月07日 07:00:14 +00:00Commented Oct 7, 2021 at 7:00
changeDot()
); Otherwise, if it isn't your code, are you asking if you can make something similar? That statement "I would like to know if I can do things like this" is slightly vague.... \$\endgroup\$