patternjavascriptMinor
Slider with jQuery using bullets
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:
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");
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
-
-
 
-  
-  
-->
-
-->
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:
You're assuming
abusing jQuery:
You may not need to use jQuery for everything: See youmightnotneedjquery.com for a full list.
For example:
Better call the redundancy department department:
Your code has quite a bit of redundancy, for example:
Could be expressed as:
And the following is also redundant:
Simply remove
Indentation and spacing:
Your indentation and spacing is wrong:
Miscellanous
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 asnodo.getElementsByTagName('ul')
$(this).data("index")can be written asthis.getAttribute('index')
$(div).parent()can be written asdiv.parentElement
.addClasscan 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
typeofvs.instanceof: While both are similar, I would prefer to useinstanceofas 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 preferNumber(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
parseIntcan cause strange issues. Unless needed, you can use10as 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.