1
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ So is the code in the fiddle your code? if so, please update your code sample so it is complete (e.g. to contain functions not defined, e.g. 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\$ – Sᴀᴍ Onᴇᴌᴀ Nov 24 '17 at 5:14
  • \$\begingroup\$ I posted only a bit of code here, the rest is on jsfiddle. \$\endgroup\$ – Davkk Nov 24 '17 at 16:40
1
\$\begingroup\$

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
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>');
});

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/

\$\endgroup\$
  • \$\begingroup\$ thank you so much for this long hardly helpful comment! wow \$\endgroup\$ – Davkk Dec 2 '17 at 9:47
  • \$\begingroup\$ You're welcome, but what do you mean by hardly? \$\endgroup\$ – Sᴀᴍ Onᴇᴌᴀ Dec 3 '17 at 14:40

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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