5
\$\begingroup\$

I have a slider with 3 images and left/right arrows. If you click on them, the image changes. You also have 3 bullets below the images and the image changes depending on what bullet you click.

I don't want to use buttons "arrows"; I want to use the tag "a." Any ideas on making it better? You'll need to add 3 images "because sliders use 3 images" and the other 2 images are to use left/right arrows (to change the images). That is what I don't want to use, as I said.

HTML:

<!DOCTYPE html>
<html>
<head>
    <title>Slider</title>
    <meta charset="utf-8" />    
    <link rel="stylesheet" href="style.css">
    <script src="js/jquery-2.1.4.min.js"></script>
    <script src="js/slider.js"></script>
</head>
<body>
<div class="sliders">
    <ul >
        <li class="activa"><img src="fotos-de-Hamburguesa-americana.jpg"></li>
        <li><img src="images (3).jpg"></li>
        <li><img src="images21312.jpg"></li>
    </ul>
<!--    <ul class="controles">
        <li class="activa">&nbsp</li>
        <li>&nbsp</li>
        <li>&nbsp</li>
    </ul> -->
</div>
<div class="sliders">
    <ul>
        <li class="activa"><img src="fotos-de-Hamburguesa-americana.jpg"></li>
        <li><img src="images (3).jpg"></li>
<!--        <li><img src="images21312.jpg"></li> -->
    </ul>   

</div>


</body>
</html>

JS:

$.fn.slider = function(config){
    var nodos = this;
    var delay = (typeof config.delay == "number")?parseInt(config.delay):4000;
    for (var i = 0; i < nodos.length; i++) {
        Slider(nodos[i]);
    }
    function Slider(nodo){
        var galeria = $(nodo).find('ul');
        var btn1 = "<button class='before'></button>";
        if(!$(nodo).hasClass('slider'))
        $(nodo).addClass('slider');

        if(!galeria.hasClass('galeria'))
        galeria.addClass("galeria");

        //Encontrar cuantas imagenes hay en la galeria
        var imagenes = $(galeria).find('li');

        //Controles
        var html_bullets="<ul class='controles'>";
        for (var it = 0; it < imagenes.length; it++) {
            if(it==0)
            html_bullets+="<li data-index='"+it+"' class='activa'>&nbsp;</li>";
            else    
            html_bullets+="<li data-index='"+it+"'>&nbsp;</li>";
        }

        html_bullets+="</ul><button class='next'></button>";
        $(nodo).append(html_bullets);
        $(nodo).prepend(btn1);
        var bullets = $(nodo).find("ul.controles li");
        bullets.click(function(){
        var index= $(this).data("index");
        bullets.removeClass('activa');
        imagenes.removeClass('activa');
        $(imagenes[index]).addClass("activa");  
        $(bullets[index]).addClass("activa");   
        });
    }
        $(".slider").on("click","button.before",function(){
            var div = this;
            div = $(div).parent();
            console.log(div);
            flechas({div:div});
        });
        $(".slider").on("click","button.next",function(){
            var div = this;
            div = $(div).parent();
            flechas({div:div,direccion:1});
        }); 

        function flechas(tipo){
            var div = tipo.div;
            var imagen = $(div).find("ul.galeria li.activa");
            var imagenes = $(div).find("ul.galeria li");
            var bullet = $(div).find("ul.controles li.activa");
            var bullets = $(div).find("ul.controles li");
            var index = bullet.data("index");
            var max = bullets.length-1;
            bullets.removeClass('activa');
            imagenes.removeClass('activa');
        if(tipo.direccion){
            if(index == max){
            $(imagenes[0]).addClass("activa");  
            $(bullets[0]).addClass("activa");   
            }else {
            $(imagenes[index+1]).addClass("activa");    
            $(bullets[index+1]).addClass("activa"); 
            }   
        }
        else{
            if(index == 0){
            $(imagenes[max]).addClass("activa");    
            $(bullets[max]).addClass("activa"); 
            }else {
            $(imagenes[index-1]).addClass("activa");    
            $(bullets[index-1]).addClass("activa"); 
            }
        }

        }   
};

$(document).ready(function() {
$(".sliders").slider({delay:5000});

});

CSS:

.slider{
    width: 420px;
    overflow: hidden;
}

.slider ul{
    list-style: none;
    padding: 0; 
}

.slider ul.galeria{
    height: 200px;
    position: relative;
    margin-left: 30px;
}

.slider ul.galeria li{
    position: absolute;
    top: 0;
    left: 0;
    opacity: 0;
    transition: all 2s;
}

.slider ul.galeria li.activa{
    opacity: 1;
}

.slider ul.galeria img{
    max-height: 200px;
    margin-left: 5px;   
}

/*Controles*/
.slider ul.controles {
    text-align: center;
}

.slider ul.controles li{
    background-color: black ;
    display: inline-block;
    width: 20px;
    height: 20px;
    border-radius: 10px;
}
.slider ul.controles li.activa{
    background-color: blue ;
}
/*Botones-flechas*/
.slider button.before{
    background-image: url(Flecha_002.png);
    position: relative;
    left: 0;
    top: 128px;
    background-repeat: no-repeat;
    background-size: 28px;
    height: 48px;
    width: 33px;
    background-color: white;
    border: none;
}
.slider button.next{
    background-image: url(Flecha_001.png);
    position: relative;
    left: 86%;
    bottom: 190px;
    background-repeat: no-repeat;
    background-size: 28px;
    height: 48px;
    width: 33px;
    background-color: white;
    border: none;
}   
.slider button.before:focus{
    outline: none;
}   
.slider button.next:focus{
    outline: none;
}
.slider button.before:hover,.slider button.next:hover,.slider ul.controles li:hover{
    cursor: pointer;
}
\$\endgroup\$
0

1 Answer 1

Reset to default
2
\$\begingroup\$

Your code is good, but there's some points you can improve upon:

Assumptions:

$.fn.slider = function(config){
    var nodos = this;
    var delay = (typeof config.delay == "number")?parseInt(config.delay):4000;

You're assuming config won't be empty or not added. You should have a default config, and simply extend the parameter config to include the items the parameter config left off.

abusing jQuery:

You may not need to use jQuery for everything: See youmightnotneedjquery.com for a full list.

For example:

  • $(nodo).find('ul') can be written as nodo.getElementsByTagName('ul')
  • $(this).data("index") can be written as this.getAttribute('index')
  • $(div).parent() can be written as div.parentElement
  • .addClass can be written as .classList.add

Better call the redundancy department department:

Your code has quite a bit of redundancy, for example:

var div = this;
div = $(div).parent();
console.log(div);
flechas({div:div});

Could be expressed as:

flechas({div: this.parentElement});

And the following is also redundant:

var btn1 = "<button class='before'></button>";
var html_bullets="<ul class='controles'>";
html_bullets+="...";
$(nodo).append(html_bullets);
$(nodo).prepend(btn1);

Simply remove btn1 and attach its contents to html_bullets.

Indentation and spacing:

Your indentation and spacing is wrong:

  • You should have spaces around your operators and after commas.
  • You should have each layer of indentation indented by either two, four or eight spaces. Additionally, keep consistent.

Miscellanous

  • typeof vs. instanceof: While both are similar, I would prefer to use instanceof as it lets you declare the type as the type, meaning the compiler will pick up if you mess up the format on the name:

if (config.delay instanceof Number)
  • parseInt(config.delay): there's many ways to do this as described by this example, however, I would prefer Number(config.delay) as it looks more clear as to what are trying to create.
    • In addition to this above point, leaving off the second parameter in parseInt can cause strange issues. Unless needed, you can use 10 as the second parameter to parse to decimal.
\$\endgroup\$
1
  • \$\begingroup\$ That's really nice information, thanks for your time and help I'll have on mind that tips \$\endgroup\$
    – JorgeAlberto
    Commented Jan 18, 2016 at 1:42

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.