5
\$\begingroup\$

I'm working on this pure JS slider. It's quite a basic one, but it's what I need. I would like your feedback on it and any improvements/features that I'm missing.

HTML

<div class="container">
<ul id="slides">
    <li class="slide showing">Slide 1</li>
    <li class="slide">Slide 2</li>
    <li class="slide">Slide 3</li>
    <li class="slide">Slide 4</li>
    <li class="slide">Slide 5</li>
</ul>
<div class="buttons">
    <button class="controls" id="previous">&lt;</button>
    <button class="controls" id="next">&gt;</button>
</div>

JS

var currentSlide = 0;
var slides = document.querySelectorAll("#slides .slide");
var controls = document.querySelectorAll(".controls");
var next = document.getElementById("next");
var previous = document.getElementById("previous");

for (var i = 0; i < controls.length; i++) {
    controls[i].style.display = "inline-block";
}

function goToSlide(n) {
    slides[currentSlide].className = "slide";
    currentSlide = (n + slides.length) % slides.length;
    slides[currentSlide].className = "slide showing";
}

function nextSlide() {
    goToSlide(currentSlide + 1);
}

function previousSlide() {
    goToSlide(currentSlide - 1);
}

next.onclick = function() {
    nextSlide();
};
previous.onclick = function() {
    previousSlide();
};

CSS

/*
Essential - Core
*/
#slides {
    position: relative;
    height: 300px;
    padding: 0px;
    margin: 0px;
    list-style-type: none;
}

.slide {
    position: absolute;
    left: 0px;
    top: 0px;
    width: 100%;
    height: 100%;
    opacity: 0;
    z-index: 1;
    -webkit-transition: opacity 1s;
    -moz-transition: opacity 1s;
    -o-transition: opacity 1s;
    transition: opacity 1s;
}

.showing {
    opacity: 1;
    z-index: 2;
}


/*
Non-essential - Styles
*/

.slide {
    font-size: 40px;
    padding: 40px;
    box-sizing: border-box;
    background: #333;
    color: #fff;
}

.slide:nth-of-type(1) {
    background: red;
}

.slide:nth-of-type(2) {
    background: orange;
}

.slide:nth-of-type(3) {
    background: green;
}

.slide:nth-of-type(4) {
    background: blue;
}

.slide:nth-of-type(5) {
    background: purple;
}

.controls {
    color: #fff;
    font-size: 40px;
    cursor: pointer;
}

.controls:hover {
    color: #333;
}

.container {
    position: relative;
}

.container button:nth-of-type(1) {
    position: absolute;
    left: 0;
    top: 50%;
    z-index: 10;
}

.container button:nth-of-type(2) {
    position: absolute;
    right: 0;
    top: 50%;
    z-index: 10;
}

The slider must be responsive. However, I didn't add any "responsiveness" code to it, because I believe that can be handled by CSS. Am I wrong?

Here's the JSFiddle

\$\endgroup\$
6
\$\begingroup\$

I would suggest wrapping this whole thing in an IIFE such that you can modularize all of your slide show functionality into its own scope.

That might look like this:

(function() {
    // your javascript code
}())

This would prevent you from defining a bunch of variables in global scope which could interact negatively with other javascript on a page where you want to insert the slide show.


I agree with other answer about removing styling for .controls into CSS. You don't use this DOM collection anywhere else in your code, so why have it in JS at all?


You might consider separating the logic for hiding/showing slides into a separate method from goToSlide(). This allows goToSlide() to just focus on setting current slide index and calling appropriate hide/show methods.


I worry about writing to className as you could clobber any other classes that had been applied to a slide element. Since it seems like your code is really only worried about adding/removing showing, consider accessing classList on the element to add/remove this class only.


I would probably place the logic related to "wrapping" index values into the next/previous convenience function and let goToSlide() do one thing only - show the slide index as given.


Consider throwing an error for an illegal slide index passed to goToSlide(), since this function is exposed publicly.


If you truly want to make this code re-usable, you may want to consider making this functionality into a proper "class" and internalize all references to state and DOM bindings. This would allow you to place multiple instances of these slide shows onto a page at once.

Putting it all together might yield something like:

// you could have this class defined in an externally included file
function Slider(slideSelector, nextId, prevId, toggleClassName) {
    this.currentIndex = 0;
    this.slides = querySelectorAll(slideSelector);
    this.next = getElementById('#' + nextId);
    this.prev = getElementById('#' + prevId);
    this.toggleClassName = toggleClassName;

    this.next.onclick = function() {
        this.nextSlide();
    }
    this.prev.onclick = function() {
        this.prevSlide();
    }
}   

Slider.prototype.goToSlide = function(idx) {
    this.validateIndex(idx);
    this.currentIndex = idx;
    this.hideAllSlides();
    this.showSlide(idx);
}

Slider.prototype.nextSlide = function() {
    var idx = this.currentIndex + 1;
    if (idx === this.slides.length) {
        idx = 0;
    }
    this.goToSlide(idx);
}

Slider.prototype.prevSlide() = function() {
    var idx = this.currentIndex - 1;
    if (idx < 0) {
        idx = this.slides.length - 1;
    }
    this.goToSlide(idx);
}

Slider.prototype.hideAllSlides = function() {
    this.slides.forEach(function(slide) {
        slides.classList.remove(this.toggleClassName);
    });
}

Slider.prototype.showSlide = function(idx) {
    this.validateIndex(idx);
    this.slides.item(idx).classList.add(this.toggleClassName);
}

Slider.prototype.validateIndex = function(idx) {
    if(!Number.isInteger(idx)) {
        throw new TypeError('Non-integer value passed');
    }
    if ((idx > this.slides.length - 1) || idx < 0) {
        throw new RangeError('Out of range index value: ' + idx);
    }  
}


// following code could then be in your document ready handler
// you could instantiate any number of sliders
// each which could operate independently of each other
var slider1 = new Slider('#slider1 .slides', 'next1', 'prev1', 'showing');
var slider2 = new Slider('#slider2 .slides', 'next2', 'prev2', 'showing');

Notes:

  • You could make ES6 class as well, but I did not show this, as I see nothing in your original code to make me think you are working in ES6.
  • Taking class approach should eliminate need for IIFE around slider code, in that you have already encapsulated your logic/state.
  • I have added index value validation, as given that the goToIndex() method could theoretically be called form anywhere in code that has access to the object in it's scope, it becomes more important to make sure object can not be put into a bad state.
\$\endgroup\$
6
  • 1
    \$\begingroup\$ Nice! Pretty much what I had in mind but way more elaborate and therefore informative. Have an up vote :) \$\endgroup\$ – Happy Koala Apr 12 '17 at 13:42
  • \$\begingroup\$ @HappyKoala - you only need the iife if you aren't bundling your code already. I have some coworkers who salt their code with iife's in very unnecessary places. If you bundle your code then the common case of maintaining scope is handled by modules and require \$\endgroup\$ – aaaaaa Apr 12 '17 at 15:28
  • \$\begingroup\$ @aaaaaa Not sure if your comment was actually intended for me. I agree with what you have said. I first proposed IIFE as a simple wrapper around OP's original code to encapsulate scope. If using the class approach, this becomes unnecessary. I will update answer to make this distinction more clear. \$\endgroup\$ – Mike Brant Apr 12 '17 at 15:35
  • \$\begingroup\$ @MikeBrant - I could have suggested an edit, but I just wanted Happy Koala to understand that iife's shouldn't be used to encapsulate modular code if he's already bundling it. And honestly the only reason I mention it is because I see it a lot at my job (the whole copy/paste what you see on stack overflow thing). I do appreciate the time you spent on the answer, and I didn't mean to discredit it! \$\endgroup\$ – aaaaaa Apr 12 '17 at 15:42
  • \$\begingroup\$ I learnt a lot today. Thank you @MikeBrant and all the others for your answers. \$\endgroup\$ – Daniel Apr 12 '17 at 15:59
2
\$\begingroup\$

Why did you choose to give the controls a display: inline-block through javascript?

I think you can omit

var controls = document.querySelectorAll(".controls");
for (var i = 0; i < controls.length; i++) {
    controls[i].style.display = "inline-block";
}

and instead just add display: inline-block to the .controls css selector. This would help you to keep logic and styling seperate.

\$\endgroup\$
0
\$\begingroup\$

Have you thought about refactoring your code into something that can be easily reused? Something like this (just a rough outline)?

function Slider(params) {
    this.currentSlide = 0;
    this.slides = document.querySelectorAll(params.slides);
    this.controls = document.querySelectorAll(params.controlsClassName);
    this.next = document.getElementById(params.nextId);
    this.previous = document.getElementById(params.previousId);
}

Slider.prototype = {

  //Put your methods here

};

var slider = new Slider({
  slides: '#slides .slide',
  controlsClassName: '.controls',
  nextId: 'next',
  previousId: 'previous'
});
\$\endgroup\$

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.