HiveBrain v1.2.0
Get Started
← Back to all entries
patternjavascriptMinor

Slider with jQuery using bullets

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
withsliderjqueryusingbullets

Problem

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:



Slider








-

-

&nbsp

  • &nbsp



  • &nbsp


-->




-
-->




JS:

`$.fn.slider = function(config){
var nodos = this;
var delay = (typeof config.delay == "number")?parseInt(config.delay):4000;
for (var i = 0; i  ";
else
html_bullets+=" ";
}

html_bullets+="";
$(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");

Solution

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 = "";
var html_bullets="";
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.

Code Snippets

$.fn.slider = function(config){
    var nodos = this;
    var delay = (typeof config.delay == "number")?parseInt(config.delay):4000;
var div = this;
div = $(div).parent();
console.log(div);
flechas({div:div});
flechas({div: this.parentElement});
var btn1 = "<button class='before'></button>";
var html_bullets="<ul class='controles'>";
html_bullets+="...";
$(nodo).append(html_bullets);
$(nodo).prepend(btn1);
if (config.delay instanceof Number)

Context

StackExchange Code Review Q#117105, answer score: 2

Revisions (0)

No revisions yet.