3
\$\begingroup\$

I'm fairly new to front end web development, but finally felt confident enough to create my own jQuery slider. I wish to receive constructive criticism about my code, how I can optimize any line of code, or ideas on how I can make this a bit more interesting.

$(document).ready(function(){
    /*
    =====================================
        THE SLIDER SCRIPT STARTS HERE 
    =====================================
    */
    var i = 0; 
    var images = ["Slide/bandera_OK%202.png","Slide/CARAVANA_carta.png","Slide/KANGOO.png","Slide/mochila.png","Slide/RADICAL_OH_03_alta.png","Slide/SILVER_final.png"];
    
    setInterval(function(){
        var topDiv = $('[data-slide='+'top'+']');
        var topImg = $('[data-slide='+'top'+'] > img');
        var bottomDiv = $('[data-slide='+'bottom'+']');
        var bottomImg = $('[data-slide='+'bottom'+'] > img');
        
        i++;
        
        if(i == images.length){
            i = 0;
             bottomDiv.animate({left:'100%'},1500);
            topImg.attr('src',images[i]);
            setTimeout(function(){
                bottomDiv.css('z-index','0');
                topDiv.css('z-index','1');
            },2000);
            
            setTimeout(function(){
                bottomDiv.css('left','0px');
                bottomImg.attr('src',images[i+1]);
            },2500);
            
        
        }
        else if(i % 2 != 0){
            topDiv.animate({left:'100%'},1500);
            bottomImg.attr('src',images[i]);
            setTimeout(function(){
                topDiv.css('z-index','0');
                bottomDiv.css('z-index','1');
            },2000);
            
            setTimeout(function(){
                topDiv.css('left','0px');
                topImg.attr('src',images[i+1]);
            },2500);
        }
        else if(i % 2 == 0){
            bottomDiv.animate({left:'100%'},1500);
            topImg.attr('src',images[i]);
            setTimeout(function(){
                bottomDiv.css('z-index','0');
                topDiv.css('z-index','1');
            },2000);
            
            setTimeout(function(){
                bottomDiv.css('left','0px');
                bottomImg.attr('src',images[i+1]);
            },2500)
            
        }
        
    },3000);
    
});
.slider {
position: relative;
}

.slider div {
position: absolute;
width: 100%;
height: 100%;
top: 0px;
left: 0px;
}

.slider div img {
width: 100%;
}

[data-slide='top'] {
z-index: 1;
}

.wrapper {
width: 100%;
height: 100%;
position: relative;
}
<div class='slider' class='wrapper'>

    <div data-slide='top'><img src="Slide/bandera_OK%202.png"></div>
    <div data-slide='bottom'><img src=""></div>

</div>

\$\endgroup\$
3
  • \$\begingroup\$ " class='slider' class='wrapper' " you can write as " class='slider wrapper' ". \$\endgroup\$ Jul 8, 2016 at 6:47
  • \$\begingroup\$ Nice work. I have brought your slider to running state on CodePen using LoremPixel-images : codepen.io/mizech/pen/akwprg Main problem I see is the appearing and disappearing horizontal slider on the bottom. No good idea how to get rid of that myself. Would interest me too. Perhaps someone else knows? \$\endgroup\$ Jul 8, 2016 at 6:50
  • \$\begingroup\$ @mizech Thanks for the feedback I will look into this and see if I can find a way to fix this, when I do I will edit the code \$\endgroup\$ Jul 8, 2016 at 18:27

1 Answer 1

2
\$\begingroup\$

The first big things I got on your code, are:

  1. you could define a function instead of passing an anonymous one to setInterval

  2. you could move the dom serarch on the main scope to avoid the repetition on each iteration.

  3. you could optimize the setTimeout function too, defining one on main scope and refer to a variable like selectedImage, instead of using the array.

Defining function, instead of using anonymous on the fly, help to avoid repeating yourself.

The "on the fly anonymous" are very good to cut off the development time, of for very short code or for examples.

But in production, it's better to came back and refactoring it.

Here how I would refactoring:

     $(document).ready(function(){
        /*
        =====================================
            THE SLIDER SCRIPT STARTS HERE 
        =====================================
        */
        var i = 0; 
        var images = ["Slide/bandera_OK%202.png","Slide/CARAVANA_carta.png","Slide/KANGOO.png","Slide/mochila.png","Slide/RADICAL_OH_03_alta.png","Slide/SILVER_final.png"];
        var topDiv = $('[data-slide='+'top'+']');
        var topImg = $('[data-slide='+'top'+'] > img');
        var bottomDiv = $('[data-slide='+'bottom'+']');
        var bottomImg = $('[data-slide='+'bottom'+'] > img');
        var selectedImage = null;

        function mySlider() {

            i++;

            if(i == images.length){
                i = 0;
                bottomDiv.animate({left:'100%'},1500);
                topImg.attr('src',images[i]);
                setTimeout(raiseTopDiv,2000);

                seletectImage = images[i+1];

                setTimeout(showSelectedImage,2500);


            }
            else if(i % 2 != 0){
                topDiv.animate({left:'100%'},1500);
                bottomImg.attr('src',images[i]);
                setTimeout(raiseBottomDiv,2000);

                seletectImage = images[i+1];

                setTimeout(showSelectedImage,2500);
            }
            else if(i % 2 == 0){
                bottomDiv.animate({left:'100%'},1500);
                topImg.attr('src',images[i]);

                setTimeout(raiseTopDiv,2000);

                seletectImage = images[i+1];

                setTimeout(showSelectedImage,2500)

            }

        }

        function raiseTopDiv() {
            bottomDiv.css('z-index','0');
            topDiv.css('z-index','1');
        }

        function raiseBottomDiv() {
            topDiv.css('z-index','0');
            bottomDiv.css('z-index','1');
        }

        function showSelectedImage() {
            bottomDiv.css('left','0px');
            bottomImg.attr('src',seletectImage);
        }

        setInterval(mySlider,3000);

    });
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

Not the answer you're looking for? Browse other questions tagged or ask your own question.