1
\$\begingroup\$

The code below is for a slideshow that shows a slide with class active, pre-loads a slide with class next, and holds the slide with class last in the background.

My biggest issue with this is there seems to be lots of duplication and probably very inefficient code here that I would like to improve upon, but do not yet have that complete knowledge.

I am not looking for the code rewritten, but would like to hear of ideas that can improve not only my knowledge of jQuery but also the efficiency of the code.

$(document).ready(function() {

  $('.slide:eq(-1)').addClass('last');
  $('.dot:first').addClass('active');
  $('.slide:first').addClass('active').delay($duration).queue(function() {
    $(this).addClass('show-text');
  });
  $('.slide:eq(1)').addClass('next');

});

// Globals
var $classes = 'last active show-text next';
var $duration = 1250;

// Function for the pagination operation
$('.dot').on('click', function() { // the dot click

  var $This = $(this);
  // Match the index to the slide numbers
  var GetIndex = $This.closest('li').index() + 1;

  $('.dot').removeClass('active').filter($This).addClass('active');

  $('.slide').dequeue();
  $('.slide').removeClass($classes);

  // Show new active slide
  $('#slide' + GetIndex).addClass('active').delay($duration).queue(function() {
    $(this).addClass('show-text');
  });
  // Add class to previous slide
  $('.slide').eq(($('.slide.active').index() - 1) % $('.slide').length).addClass('last');
  // Add class to next slide
  $('.slide').eq(($('.slide.active').index() + 1) % $('.slide').length).addClass('next');

});

$('.button').click(function moveSlide() {

  // Variables for moving slide
  var $active = $('.slide.active');
  var $prevSlide = $('.slide').eq(($active.index() - 1) % $('.slide').length);
  var $afterPrevSlide = $('.slide').eq(($active.index() - 2) % $('.slide').length);
  var $nextSlide = $('.slide').eq(($active.index() + 1) % $('.slide').length);
  var $slideAfterNext = $('.slide').eq(($active.index() + 2) % $('.slide').length);

  // Variables for pagination
  var $tagNextDot = $('#' + $prevSlide.attr('id') + 'Dot');
  var $tagPrevDot = $('#' + $nextSlide.attr('id') + 'Dot');

  $active.dequeue();
  $('.slide').removeClass($classes)
  $('.dot').removeClass('active');

  if ($(this).is("#prev")) {

    $active.addClass('next');
    $tagNextDot.addClass('active');
    $prevSlide.addClass('active').delay($duration).queue(function() {
      $(this).addClass('show-text');
    });
    $afterPrevSlide.addClass('last');

  } else {
    $active.addClass('last');
    $tagPrevDot.addClass('active');
    $nextSlide.addClass('active').delay($duration).queue(function() {
      $(this).addClass('show-text');
    });
    $slideAfterNext.addClass('next');
  }

});
body {
  font-size: 16px;
  font-family: 'Heebo', sans-serif;
  text-transform: uppercase;
  font-weight: 900;
}


/* Slides */

.slide-wrapper {
  position: absolute;
  top: 0;
  left: 0;
  bottom: 0;
  right: 0;
  display: flex;
  overflow: hidden;
}

.slide {
  position: absolute;
  display: flex;
  justify-content: center;
  align-items: center;
  height: 100vh;
  width: 70%;
  left: 140%;
  z-index: 0;
  transition: 1.25s;
  box-shadow: -10px 0px 21px -5px rgba(0, 0, 0, 0.5);
}

.slide h2 {
  display: none;
  color: #fff;
  text-shadow: 0px 0px 8px rgba(0, 0, 0, 0.5);
  letter-spacing: -2px;
  font-size: 3rem;
}

.slide.active.show-text h2 {
  display: block;
  animation: reveal-text 1.5s forwards;
}

@keyframes reveal-text {
  0% {
    opacity: 0;
  }
  100% {
    opacity: 1;
  }
}

#slide1 {
  background: linear-gradient(to right, #ff416c, #ff4b2b);
}

#slide2 {
  background: linear-gradient(to right, #00b4db, #0083b0);
}

#slide3 {
  background: linear-gradient(to right, #59c173, #a17fe0, #5d26c1);
}

#slide4 {
  background: linear-gradient(to right, #ad5389, #3c1053);
}

.slide.last {
  left: 0;
  z-index: 0;
}

.slide.active {
  left: 0;
  z-index: 1;
}

.slide.next {
  left: 70%;
  z-index: 2;
}

.dots-wrapper {
  z-index: 10;
  list-style: none;
  padding-left: 0;
  position: absolute;
  bottom: 0;
  padding: 10px;
}

.dots-wrapper li {
  display: inline;
}

.dot {
  display: inline-block;
  width: 8px;
  height: 8px;
  border: 2px solid #fff;
  border-radius: 6px;
}

.dot.active {
  background-color: red;
  border-color: red;
}


/* Buttons */

.button-wrapper {
  display: flex;
  z-index: 10;
  width: 100%;
  justify-content: space-between;
  align-items: center;
}

.button {
  background-color: rgba(0, 0, 0, 0.45);
  color: #ddd;
  height: 40px;
  border: none;
  font-weight: bold;
  padding: 10px 20px;
  transition: 0.3s;
}

.button:hover {
  cursor: pointer;
  background: rgba(0, 0, 0, 0.85);
  color: #fff;
}
<script src="https://cdnjs.cloudflare.com/ajax/libs/jquery/3.3.1/jquery.min.js"></script>
<div class="slide-wrapper">
  <div id="slide1" class="slide">
    <h2>Slide One.</h2>
  </div>
  <div id="slide2" class="slide">
    <h2>Slide Two.</h2>
  </div>
  <div id="slide3" class="slide">
    <h2>Slide Three.</h2>
  </div>
  <div id="slide4" class="slide">
    <h2>Slide Four.</h2>
  </div>
  <div class="button-wrapper">
    <button id="prev" class="button">Prev.</button>
    <button id="next" class="button">Next.</button>
  </div>
  <ul class="dots-wrapper">
    <li>
      <span id="slide1Dot" class="dot"></span>
    </li>
    <li>
      <span id="slide2Dot" class="dot"></span>
    </li>
    <li>
      <span id="slide3Dot" class="dot"></span>
    </li>
    <li>
      <span id="slide4Dot" class="dot"></span>
    </li>
  </ul>
</div>

\$\endgroup\$
3
+50
\$\begingroup\$

Overall, I would say that the code is okay but the biggest thing that stands out is that it has A LOT of DOM queries, and while that may not slow things down much for a small DOM, it isn't very efficient. There are also some redundant callback functions that can be abstracted to a named function - e.g. the function that adds the class show-text to an element.


As of the latest version of jQuery, the recommended syntax for the DOM ready callback has been simplified to just $(function() {...})1.


Last year I read this article about optimizing JavaScript code and suggest that you take a look at it if you haven't already. I know it has a negative tone towards jQuery so ignore that if you plan to keep going with jQuery. The third sub-heading of that article is Cache DOM Lookups. You could store $('.slide') in a variable in the DOM-ready callback, as well as $('.dot') and $('.button') and use those when manipulating the DOM later.


If you cache those jQuery collections then you can utilize jQuery methods like .first(), .last(), .eq() to filter the existing collections to particular nodes instead of running new DOM queries.

That means that these lines:

$(document).ready(function() {

  $('.slide:eq(-1)').addClass('last');
  $('.dot:first').addClass('active');
  $('.slide:first').addClass('active').delay($duration).queue(function() {
    $(this).addClass('show-text');
  });
  $('.slide:eq(1)').addClass('next');

Could be transformed like this:

var slides, dots, buttons;
$(function() { //DOM ready callback 
  slides = $('.slide');
  dots = $('.dot');
  buttons = $('.button');

  slides.last().addClass('last');   
  dots.first().addClass('active');  
  slides.first().addClass('active').delay($duration).queue(function() {
    $(this).addClass('show-text');
  });                               
  slides.eq(1).addClass('next');    

Those cached collections can also be used when adding the clock handlers to the elements with class names dot and button, which should also happen after the DOM is ready.

  buttons.click(function moveSlide() {...});
  dots.click(function() {...});

As far as associating a dot with a slide, you could utilize data attributes and then take advantage of the jQuery method .data() to get/set such values, but utilizing .index() should be sufficient.


As was mentioned above, the function to add class show-text to an element can be abstracted to a named function:

function addShowTextClass() {
  $(this).addClass('show-text');
}

Then that can be used in places where that function appears - e.g. instead of

$('.slide:first').addClass('active').delay($duration).queue(function() {
  $(this).addClass('show-text');
});

That can be updated to the following:

$('.slide:first').addClass('active').delay($duration).queue(addShowTextClass);

As well as for this line in the dot click handler:

$('#slide' + GetIndex).addClass('active').delay($duration).queue(function() {
  $(this).addClass('show-text');
});

And two more occurrences in the moveSlide() function can be updated as well.


The constants could be declared using the keyword const, unless you need to support browsers that don't support that keyword. A common convention in JavaScript and many other languages is to use all uppercase letters for constants. While there isn't anything wrong with starting variables with a dollar sign, that is typically only found with jQuery source code and plugins, but not required. If you prefer to keep those then go ahead.


The click handlers are added in two forms -

$('.dot').on('click', function() { // the dot click

and

$('.button').click(function moveSlide() {

There isn't anything wrong with using the shortcut in one place but not the other, though it is inconsistent. Why not use the same format in both places? Also, why name one but not the other?

I would suggest moving the functions out and then after assigning the DOM collections to variables in the DOM ready callback, add the click handlers using named functions:

var dots, slides, buttons;

$(function() { //DOM ready callback - see next section for explanation
  slides = $('.slide');
  dots = $('.dot');
  buttons = $('.button');

  dots.click(dotClickHandler);
  buttons.click(moveSlide);

});

function dotClickHandler() { ...}
function moveSlide() {...}

This line can be eliminated:

var $This = $(this);

while that variable is used twice, it doesn't hurt anything to replace the two instances with $(this)...


Lastly, there are quite a few posts on this site involving jquery slideshows. It may help you to look at some of those as well.

Rewrite

The code below uses the advice above. The id attributes were removed from the dots and the same could likely be done to the slide elements if desired.

// Globals
const CLASSES = 'last active show-text next';
const DELAY_DURATION = 1250;

function addShowTextClass() {
  $(this).addClass('show-text');
}
var dots, slides, buttons;

$(function() { //DOM ready callback - simplified format which is recommended in jQuery documentation
  slides = $('.slide');
  dots = $('.dot');
  buttons = $('.button');

  slides.last().addClass('last');
  dots.first().addClass('active');
  slides.first().addClass('active').delay(DELAY_DURATION).queue(addShowTextClass); 
  slides.eq(1).addClass('next');

  dots.click(dotClickHandler);
  buttons.click(moveSlide);
});


// Function for the pagination operation
function dotClickHandler() { // the dot click

  //var $This = $(this);
  // Match the index to the slide numbers
  var listIndex = $(this).closest('li').index();

  dots.removeClass('active').filter($(this)).addClass('active');

  slides.dequeue();
  slides.removeClass(CLASSES);

  // Show new active slide
  slides.eq( listIndex).addClass('active').delay(DELAY_DURATION).queue(addShowTextClass);
  // Add class to previous slide
  slides.eq((slides.filter('.active').index() - 1) % slides.length).addClass('last');
  // Add class to next slide
  slides.eq(( slides.filter('.active').index() + 1) % slides.length).addClass('next');

}

function moveSlide() {

  // Variables for moving slide
  var $active = slides.filter('.active');
  var $prevSlide = slides.eq(($active.index() - 1) % slides.length);
  var $afterPrevSlide = slides.eq(($active.index() - 2) % slides.length);
  var $nextSlide = slides.eq(($active.index() + 1) % slides.length);
  var $slideAfterNext = slides.eq(($active.index() + 2) % slides.length);

  // Variables for pagination
  var $tagNextDot = dots.eq($prevSlide.index()); 
  var $tagPrevDot = dots.eq($nextSlide.index()); 

  $active.dequeue();
  slides.removeClass(CLASSES)
  dots.removeClass('active');

  if ($(this).is("#prev")) {

    $active.addClass('next');
    $tagNextDot.addClass('active');
    $prevSlide.addClass('active').delay(DELAY_DURATION).queue(addShowTextClass);
    $afterPrevSlide.addClass('last');

  } else {
    $active.addClass('last');
    $tagPrevDot.addClass('active');
    $nextSlide.addClass('active').delay(DELAY_DURATION).queue(addShowTextClass);
    $slideAfterNext.addClass('next');
  }
}
body {
  font-size: 16px;
  font-family: 'Heebo', sans-serif;
  text-transform: uppercase;
  font-weight: 900;
}


/* Slides */

.slide-wrapper {
  position: absolute;
  top: 0;
  left: 0;
  bottom: 0;
  right: 0;
  display: flex;
  overflow: hidden;
}

.slide {
  position: absolute;
  display: flex;
  justify-content: center;
  align-items: center;
  height: 100vh;
  width: 70%;
  left: 140%;
  z-index: 0;
  transition: 1.25s;
  box-shadow: -10px 0px 21px -5px rgba(0, 0, 0, 0.5);
}

.slide h2 {
  display: none;
  color: #fff;
  text-shadow: 0px 0px 8px rgba(0, 0, 0, 0.5);
  letter-spacing: -2px;
  font-size: 3rem;
}

.slide.active.show-text h2 {
  display: block;
  animation: reveal-text 1.5s forwards;
}

@keyframes reveal-text {
  0% {
    opacity: 0;
  }
  100% {
    opacity: 1;
  }
}

#slide1 {
  background: linear-gradient(to right, #ff416c, #ff4b2b);
}

#slide2 {
  background: linear-gradient(to right, #00b4db, #0083b0);
}

#slide3 {
  background: linear-gradient(to right, #59c173, #a17fe0, #5d26c1);
}

#slide4 {
  background: linear-gradient(to right, #ad5389, #3c1053);
}

.slide.last {
  left: 0;
  z-index: 0;
}

.slide.active {
  left: 0;
  z-index: 1;
}

.slide.next {
  left: 70%;
  z-index: 2;
}

.dots-wrapper {
  z-index: 10;
  list-style: none;
  padding-left: 0;
  position: absolute;
  bottom: 0;
  padding: 10px;
}

.dots-wrapper li {
  display: inline;
}

.dot {
  display: inline-block;
  width: 8px;
  height: 8px;
  border: 2px solid #fff;
  border-radius: 6px;
}

.dot.active {
  background-color: red;
  border-color: red;
}


/* Buttons */

.button-wrapper {
  display: flex;
  z-index: 10;
  width: 100%;
  justify-content: space-between;
  align-items: center;
}

.button {
  background-color: rgba(0, 0, 0, 0.45);
  color: #ddd;
  height: 40px;
  border: none;
  font-weight: bold;
  padding: 10px 20px;
  transition: 0.3s;
}

.button:hover {
  cursor: pointer;
  background: rgba(0, 0, 0, 0.85);
  color: #fff;
}
<script src="https://cdnjs.cloudflare.com/ajax/libs/jquery/3.3.1/jquery.min.js"></script>
<div class="slide-wrapper">
  <div id="slide1" class="slide">
    <h2>Slide One.</h2>
  </div>
  <div id="slide2" class="slide">
    <h2>Slide Two.</h2>
  </div>
  <div id="slide3" class="slide">
    <h2>Slide Three.</h2>
  </div>
  <div id="slide4" class="slide">
    <h2>Slide Four.</h2>
  </div>
  <div class="button-wrapper">
    <button id="prev" class="button">Prev.</button>
    <button id="next" class="button">Next.</button>
  </div>
  <ul class="dots-wrapper">
    <li>
      <span class="dot"></span>
    </li>
    <li>
      <span class="dot"></span>
    </li>
    <li>
      <span class="dot"></span>
    </li>
    <li>
      <span class="dot"></span>
    </li>
  </ul>
</div>

1https://api.jquery.com/ready

\$\endgroup\$
  • 1
    \$\begingroup\$ This is excellent. Thank you. This was the type of explanation I was looking for. I am yet to implement it fully but this has given me the extra bit of knowledge I was looking for. \$\endgroup\$ – Bagseye Jan 28 at 17:10

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.