2
\$\begingroup\$

I built this simple slider with JS, but I am not so experienced with programming and I wonder what can be improved in this code, how to dry it, or how to apply some OOP paradigms to it, what is not good in it, what is bad practice... If you are going to build this simple slider how would you approach it?

JSFIDDLE

HTML

<div class="container">
    <button id="prev">Prev</button>
    <!-- -->
    <button id="next">Next</button>
    <div class="slider-wrapper">
        <ul>
            <li>
                <img src="http://www.lorempixel.com/300/200" alt="">
            </li>
            <li>
                <img src="http://www.lorempixel.com/300/200/animals" alt="">
            </li>
            <li>
                <img src="http://www.lorempixel.com/300/200/city" alt="">
            </li>
            <li>
                <img src="http://www.lorempixel.com/300/200/people" alt="">
            </li>
            <li>
                <img src="http://www.lorempixel.com/300/200" alt="">
            </li>
            <li>
                <img src="http://www.lorempixel.com/300/200/animals" alt="">
            </li>
            <li>
                <img src="http://www.lorempixel.com/300/200/city" alt="">
            </li>
            <li>
                <img src="http://www.lorempixel.com/300/200/people" alt="">
            </li>
        </ul>
    </div>
</div>

CSS

* {
    margin: 0;
    padding: 0;
    border: 0;
    list-style-type: none;
    transition: all .5s;
}
body {
    font: bold 100%/1.5 monospace;
}
.container {
    width: 300px;
    height: 200px;
    margin: auto;
    position: relative;
}
.slider-wrapper {
    overflow: hidden;
}
.slider-wrapper ul {
    width: calc(8 * 300px);
    overflow: hidden;
}
.slider-wrapper li {
    float: left;
}
img {
    display: block;
}
button {
    font: bold 80%/1.5 monospace;
    cursor: pointer;
    padding: 1em;
    background-color: purple;
    color: whitesmoke;
    border-radius: 50%;
    position: absolute;
    top: 35%;
    z-index: 5;
}
#prev {
    left: -50px;
}
#next {
    right: -50px;
}

JS

    //window.addEventListener('load',function(){
var imgs = document.getElementsByTagName('img');
var numOfImgs = imgs.length; // 4
var widthOfImg = 300; //imgs[0].getBoundingClientRect().width; // 300
var prevBtn = document.getElementById('prev');
var nextBtn = document.getElementById('next');
var currentImg = 0;
var sliderWrapper = document.querySelector('.slider-wrapper>ul');
var translatedPixels = 0;


prevBtn.addEventListener('click', function () {
    if (currentImg === 0) {
        return;
    }
    currentImg--;
    sliderWrapper.style.transform = 'translateX(' + (-translatedPixels + widthOfImg) + 'px)';
    translatedPixels -= 300;
});
nextBtn.addEventListener('click', function () {
    if (currentImg === numOfImgs - 1) {
        return;
    }
    currentImg++;
    sliderWrapper.style.transform = 'translateX(' + -widthOfImg * currentImg + 'px)';
    translatedPixels += 300;
});
//});
\$\endgroup\$
2
\$\begingroup\$
* {
    margin: 0;
    padding: 0;
    border: 0;
    list-style-type: none;
    transition: all .5s;
}

First, I wouldn't do this. This resets styles on all elements, including other elements on the page. One rule I follow in CSS is not to unnecessarily override styles, especially the defaults. Unless you have a good reason to do so or you have custom base style for everything (like bootstrap).

.gallery{...}
.gallery .button{...}
.gallery .items{...}

//or

.gallery{...}
.gallery-button{...}
.gallery-items{...}

Another good practice in CSS is to namespace your styles. Selectors after another selector only take effect to descendants of the former selector. This makes it possible to limit the coverage of the style down to a certain element's descendants. Another way to do it is to use a naming scheme that's similar to namespacing. This makes your styles mean something, appear like it has heirarchy, and does not accidentally override other elements' styles.

<div class="gallery"
  <ul>
    <li><img src="" /></li>
    <li><img src="" /></li>
    <li><img src="" /></li>
  </ul>
</div>

The markup can also be simplified. A slider is simply a list of things. The markup should be built in a way that it does resemble a list semantically. Then you improve using JS from there. You can add the buttons dynamically in JS, as they mean nothing in the markup.

;(function(){

 // Your code here

}());

One thing I find annoying in JS is that everything you write is exposed to the global namespace by default. Your code will be prone to conflicting names, or mutating values. One thing you can do to prevent being a victim, or being the cause of conflicts is to enclose your code inside a closure. This prevents your code from being overwritten. To prevent your code overwriting stuff, you must declare stuff in this scope (use var).

var imageWidth = 300;

function generateXTranslate(index, width){
  return 'translateX(-' + (index * width) + 'px)';
}

nextBtn.addEventListener('click', function () {
  if (index === numOfImgs - 1) return;
  sliderWrapper.style.transform = generateXTranslate(++index, imageWidth);
});

prevBtn.addEventListener('click', function () {
  if (!index) return;
  sliderWrapper.style.transform = generateXTranslate(--index, imageWidth);
});

A lot of the code is redundant. You can simplify by taking out common code and moving them out to their function. In here, you can move out the setting of the transform into a function. Also, you can simply track down the current image as an index instead of the pixels, since the translated pixels is just a function of the index times the width of the images.

| improve this answer | |
\$\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.