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

jQuery responsive tabs to accordion

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

Problem

I wrote a script that adapts HTML tabs for different screen resolutions. On a small screen it's an Accordion and on wider ones they're Tabs. The script is for a production site, and I'm not so good in JavaScript yet, so show me any weaknesses.



`(function ($) {

// html structure
// div - selector
// // ul
// // // li.class - tabs

$.fn.tabslight = function () {
var classes = {
tabsTrigger: '.tab__trigger',
tabTypeAccordeon: 'tab-type-accordeon',
tabTypeTab: 'tab-type-tabs',
activeClass: 'current',
tabList: 'tab-list',
tabListItem: 'tab-list__item',
tabContent: 'tab-content',
tabContentItem: 'tab-content__item',
tabContent: 'tab__inner'
},
slideDuration = 300, //ms
container = $(this),
contentItems = container.find('.' + classes.tabContentItem),
isAccordeon = true,
tabsUpdate = function() {
var items = $('.' + classes.tabContentItem),
itemsTriger = $('.' + classes.tabListItem),
itemsContent = container.find('.' + classes.tabContent);
isAccordeon = ( $(window).width() ' + item + '';
});
string += '';

container.prepend(string);// Prepend list tabs for desktop statemenm

tabsUpdate(); // First run tabs update function.
}(),
triggers = container.find(classes.tabsTrigger);

// Listen for resize, and update tabs type
window.addEventListener('resize', tabsUpdate);

triggers.on('click', function (e) {

e.preventDefault(); // Disable the default action on clicking item
if ( isAccordeon ) { // if accordeon

var item = $(this).parent(),
itemContent = item.find('.' + classes.tabContent),
items = item.siblings();
itemsContent = items.find('.' + classes.tabContent);

if ( ! item.hasClass( classes.activeClass ) ) { // if elem don't has current class
items.removeClass(classes.activeClass);
item.addClass(classes.activeClass);
itemsContent.stop(true, true).slideUp(slideDuration); // hide siblings content
itemContent.stop(true

Solution

This is a partial review
Comma operator

Don't use the comma operator. Besides that it is a maintenance hell, and increases the amount of variables that are accidentally dropped in the global namespace, it serves no practical purpose here. On top of that, you even use it outside var statements, creating something that is even more incomprehensible to review.

Your use of the comma operator also messes with the indentation you are using, making it harder to read your code.

var itemTrigger = $(this).parent(),
                itemTriggerSiblings = itemTrigger.siblings(),
                itemTriggerNumber = itemTrigger.index(),
                item = container.find('.' + classes.tabContentItem).eq(itemTriggerNumber);
                itemContent = item.find('.' + classes.tabContent),
                items = item.siblings();
                itemsContent = items.find('.' + classes.tabContent);


In the code above you have a semicolon after defining all variables. Then have two more statements seperated by a comma, with a semicolon. Then another statement with a semicolon. Why? Just use the semicolon (;) after each statement.
Naming

Decide on a consistent naming scheme for your classes. It makes little sense to use both the - and the __ separator in those class names.

Code Snippets

var itemTrigger = $(this).parent(),
                itemTriggerSiblings = itemTrigger.siblings(),
                itemTriggerNumber = itemTrigger.index(),
                item = container.find('.' + classes.tabContentItem).eq(itemTriggerNumber);
                itemContent = item.find('.' + classes.tabContent),
                items = item.siblings();
                itemsContent = items.find('.' + classes.tabContent);

Context

StackExchange Code Review Q#149114, answer score: 2

Revisions (0)

No revisions yet.