First attempt at a pure JavaScript slider

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

    Slide 1
    Slide 2
    Slide 3
    Slide 4
    Slide 5

    <
    >

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: rela

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

StackExchange Code Review Q#160507, answer score: 6