patternjavascriptMinor
jQuery accordion list
Viewed 0 times
listjqueryaccordion
Problem
I'd like some tips on optimizing the following code.
I'm a bit concerned that I'm 'polluting the global namespace' by not wrapping it in a function. The commented out
I'm a bit concerned that I'm 'polluting the global namespace' by not wrapping it in a function. The commented out
var rotator { init { came from a JavaScript tutorial but I'm not sure it's the jQuery way? Should I wrap it in $(function()... instead?$(document).ready(function() {
//Promos rotation
var $accordionList = $('.accordion').find('li');
// var rotator = {
// init: function() {
var numberOfItems = $accordionList.length;
var currentItem = 0;
// Set first item to active
$accordionList.eq(currentItem).addClass('active').find('.content').slideToggle(800, function() {});
var infiniateLoop = setInterval(function() {
if(currentItem == numberOfItems - 1){
currentItem = 0;
}
else {
currentItem++;
}
// Remove active class, if is has it, and close content
$accordionList.parent().find('li.active').removeClass('active')
.find('.content').slideToggle(800, function() {
});
// Add active class and open content
$accordionList.eq(currentItem).addClass('active').find('.content').slideToggle(800, function() {
});
}, 4000 );
//$('.accordion li').on('click', function () {
$accordionList.on('click', function () {
// Stop rotation
clearInterval(infiniateLoop);
var $accordionHead = $(this);
// Remove active class, if is has it, and close content
$accordionHead.parent().find('li.active').removeClass('active')
.find('.content').slideToggle(800, function() {
});
// Add active class and open content
$accordionHead.addClass('active').find('.content').slideToggle(800, function() {
});
});
// }
// };
// rotator.init();
});Solution
To your question ( confirmed by the comments ), you are not polluting the namespace because you are
Furthermore, from what I can see that is left over from
From a once over:
- Wrapping already inside
$(document).ready(function() {
- Using
varfor every variable.
Furthermore, from what I can see that is left over from
rotator, it was probably a better approach than what you have now.From a once over:
- Spelling is important: infiniateLoop -> infiniteLoop
slideToggle(800, function() {});can simply be written asslideToggle(800);
- You could use a ternary for increasing
currenItem:currentItem = currentItem == numberOfItems - 1 ? 0 : ++currentItem;
- Please remove commented out code, also please re-indent after taking out
rotator
- It is considered good practice to have one big var at the start instead of multiple vars:
var $accordionList = $('.accordion').find('li'),
numberOfItems = $accordionList.length,
currentItem = 0;
// Set first item to active
$accordionList.eq(currentItem).addClass('active').find('.content').slideToggle(800);- Jshint could not find a single flaw
Code Snippets
var $accordionList = $('.accordion').find('li'),
numberOfItems = $accordionList.length,
currentItem = 0;
// Set first item to active
$accordionList.eq(currentItem).addClass('active').find('.content').slideToggle(800);Context
StackExchange Code Review Q#19331, answer score: 3
Revisions (0)
No revisions yet.