7
\$\begingroup\$

To learn JavaScript I made this slider. It works good but I can see myself that there is a lot of repeating of the same code. How can I rewrite it?

Codepen

  var slides = document.querySelectorAll('.slider .slide'),
  currentSlide = 0,
  firstBtn = document.getElementById('one'),
  secondBtn = document.getElementById('two'),
  thirdBtn = document.getElementById('three');

function getAllSiblings(elem, filter) {
    var sibs = [];
    elem = elem.parentNode.firstChild;
    do {
            if (elem.nodeType === 3) continue; // text node
        if (!filter || filter(elem)) sibs.push(elem);
    } while (elem = elem.nextSibling)
    return sibs;
}

function nextSlide() {
  slides[currentSlide].className = 'slide';
  currentSlide = (currentSlide + 1) % slides.length;
  slides[currentSlide].className = 'slide active';
};

firstBtn.onclick = function() {
  var slide = getAllSiblings(slides[0]);
  slide.forEach(function(el){
    el.classList.remove('active');
  });
  slides[0].className = 'slide active';
var result = getAllSiblings(this);
  result.forEach(function(el){
    el.classList.remove('active');
  })
  this.classList.add('active');

};
secondBtn.onclick = function() {
  var slide = getAllSiblings(slides[1]);
  slide.forEach(function(el){
    el.classList.remove('active');
  });
  slides[1].className = 'slide active';
var result = getAllSiblings(this);
  result.forEach(function(el){
    el.classList.remove('active');
  })
  this.classList.add('active');

};
thirdBtn.onclick = function() {
 var slide = getAllSiblings(slides[2]);
  slide.forEach(function(el){
    el.classList.remove('active');
  });
  slides[2].className = 'slide active';
var result = getAllSiblings(this);
  result.forEach(function(el){
    el.classList.remove('active');
  })
  this.classList.add('active');
};
\$\endgroup\$
2

1 Answer 1

5
\$\begingroup\$

Functions

You could create a function slide with two parameters. The first (slideIndex) is the number of the button you've clicked. The second (that) is the element you've clicked on. If you call the function, that must be equal to this.

The methodes getAllSiblings and nextSlide I've removed because if you hold the current slide (currentSlide) and the active button (activeBtn) in variables, it's not needed to get the next or previous sibling elements. Just change this after the click event is triggered by using this peudocode:

var active; // Contains an active button

document.getElementById('clickbutton').addEventListener('click', function() {
    // Remove the class of the active button.
    active = this; // Set active to new button.
    // Add active class by the triggered element. You could access that by the `this` 
    // keyword.
});

After removing the active class of the current slide and active button, the variables currentSlide and activeBtn must been set to the new slide and must the class active been added to the new nodes.

The function init I've made for initialize the components you needed. This I've done to look just one time for each component inside the HTML code and because of this, the lookups are reduced to four times in total.

Use strict

The "use strict"; defines that JavaScript code should be executed in "strict mode". This means that code below will give an error:

"use strict";
x = 3.14;         // --> here is the error
console.log(x);

Uncaught ReferenceError: x is not defined.

Code below (without use strict) will give the output 3.14.

x = 3.14;
console.log(x);

More information about this on W3Schools (JavaScript Use Strict).

Performance

I've look to make your JS more performance.

  • I've limit the DOM (Domain Object Model) updates and lookups. Use so less as you can the functions document.getElement(s)By... and document.querySelector(all) or DOM updates, like setting a style using the style property or setAttribute methode.
  • Used functions for repeating code. This will keep your code DRY (Don't Repeat Yourself).

More information you could find on Stack Overflow documentation: Performance tips.

Minimize

If your site goes online for public usage, minify your code. This means that your Javascript and CSS code will be rewritten without enters, not needed white spaces, shorter variables names and not important remarks. Dependent of the original size of your code, it could be 50% till 60% compressed. This results in a faster download of your webpage. In your case it would be compressed by 49.84%, saving 0.59 kb for the JavaScript code.

Good tools to do this are

CSS optimization

It's not needed to write units when the value is zero in CSS. Use this instead:

.foo {
    margin: 0;
}

Full code

Below you could find the new code or on this CodePen:

"use strict";                                      // --> JS executed in strict mode
var slides = [],
    currentSlide = 0,                              // --> current slide
    activeBtn;                                     // --> active button

function slide(slideIndex, that) {
  activeBtn.classList.remove('active');            // --> remove active class
  slides[currentSlide].classList.remove('active'); //     from "old" slide

  currentSlide = slideIndex;                       // --> set new slide and 
  activeBtn = that;                                //     active button

  activeBtn.classList.add('active');
  slides[currentSlide].classList.add('active');
}

function init() {
  slides = document.getElementsByClassName('slide');
  var firstBtn = document.getElementById('one'),
      secondBtn = document.getElementById('two'),
      thirdBtn = document.getElementById('three');

  activeBtn = firstBtn;

  firstBtn.addEventListener('click', function() {
    slide(0, this);                                 // --> call function to slide
  });
  secondBtn.addEventListener('click', function() {
    slide(1, this);
  });
  thirdBtn.addEventListener('click', function() {
    slide(2, this);
  });
}

init();
html,
body {
  height: 100%;
  width: 100%;
}
.slider {
  position: relative;
  height: 300px;
  padding: 0;
  margin: 0;
  list-style-type: none;
}
.slide {
  position: absolute;
  left: 0;
  top: 0;
  width: 100%;
  height: 100%;
  transition: all 2s ease-in-out;
  background: #000;
  opacity: 0;
}
.active {
  opacity: 1;
}
.slide:first-child {
  background: tomato;
}
.slide:nth-child(2) {
  background: green;
}
.controllers {
  width: 80px;
  height: 20px;
  display: flex;
  background: #000;
}
.controllers span {
  display: block;
  background: white;
  height: 20px;
  width: 20px;
  border-radius: 50%;
  margin-right: 5px;
}
.controllers span.active {
  background: green;
}
<div class="slider">
  <div class="slide active"></div>
  <div class="slide"></div>
  <div class="slide"></div>
</div>

<div class="controllers">
  <span id='one' class='active'></span>
  <span id='two'></span>
  <span id='three'></span>
</div>

\$\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.