patternjavascriptMinor
Accessible tabbed UI
Viewed 0 times
accessibletabbedstackoverflow
Problem
I've started working on a script that turns sections with headings into a tabbed interface.
The repo is at https://github.com/derekjohnson/tabs and a demo at http://derekjohnson.github.io/tabs/
It works in IE8 up, although I haven't fully tested it on all the devices I have access to.
I'm particularly interested in feedback on the tabs.js file, and welcome all comments.
As I mentioned in the README I know it could do with a config object, but I've never done one so I'll be learning on this project. Any tips on that gratefully received too.
tabs.js:
```
(function(win, doc, undefined) {
'use strict';
// Quick feature test
if('querySelector' in doc) {
var tabs = function() {
/* Helper functions
========================================================================== */
// Cross browser events
var add_event = function(el, ev, fn) {
'addEventListener' in win ?
el.addEventListener(ev, fn, false) :
el.attachEvent('on' + ev, fn);
};
// Faster class selectors
// http://jsperf.com/queryselector-vs-getelementsbyclassname-0
var get_single_by_class = function(className) {
return 'getElementsByClassName' in doc ?
doc.getElementsByClassName(className)[0] :
doc.querySelector('.' + className);
}
//http://jsperf.com/byclassname-vs-queryselectorall
var get_many_by_class = function(className) {
return 'getElementsByClassName' in doc ?
doc.getElementsByClassName(className) :
doc.querySelectorAll('.' + className);
}
/* Feature detect for localStorage courtesy of
http://mathiasbynens.be/notes/localstorage-pattern
========================================================================== */
var storage,
fail,
uid;
try {
uid = new Date;
(storage
The repo is at https://github.com/derekjohnson/tabs and a demo at http://derekjohnson.github.io/tabs/
It works in IE8 up, although I haven't fully tested it on all the devices I have access to.
I'm particularly interested in feedback on the tabs.js file, and welcome all comments.
As I mentioned in the README I know it could do with a config object, but I've never done one so I'll be learning on this project. Any tips on that gratefully received too.
tabs.js:
```
(function(win, doc, undefined) {
'use strict';
// Quick feature test
if('querySelector' in doc) {
var tabs = function() {
/* Helper functions
========================================================================== */
// Cross browser events
var add_event = function(el, ev, fn) {
'addEventListener' in win ?
el.addEventListener(ev, fn, false) :
el.attachEvent('on' + ev, fn);
};
// Faster class selectors
// http://jsperf.com/queryselector-vs-getelementsbyclassname-0
var get_single_by_class = function(className) {
return 'getElementsByClassName' in doc ?
doc.getElementsByClassName(className)[0] :
doc.querySelector('.' + className);
}
//http://jsperf.com/byclassname-vs-queryselectorall
var get_many_by_class = function(className) {
return 'getElementsByClassName' in doc ?
doc.getElementsByClassName(className) :
doc.querySelectorAll('.' + className);
}
/* Feature detect for localStorage courtesy of
http://mathiasbynens.be/notes/localstorage-pattern
========================================================================== */
var storage,
fail,
uid;
try {
uid = new Date;
(storage
Solution
From a once over:
-
The bottom part of your code is too stretched vertically, this:
could have been this:
-
I am pretty sure this
could be
-
The bigger picture is that this:
could be
On the whole, I think you should review your code to reduce the line count. I am not saying that you should play CodeGolf, but this could be written in a fewer lines while keeping the same level of quality.
- In general, impressive code
-
The bottom part of your code is too stretched vertically, this:
/* Insert the tabs into the DOM
========================================================================== */
tabs.appendChild(frag);
wrapper.insertBefore(tabs, get_single_by_class('js-panel'));
/* Listen for clicks on the tab list
========================================================================== */
add_event(tabs, 'click', clicked);
/* Listen for key presses
========================================================================== */
add_event(tabs, 'keydown', kbd);
/* If a tab id is in localStorage open the corresponding panel
========================================================================== */
if(storage && localStorage['tab']) {
show_hide(localStorage['tab']);
}
};
// Make all that happen
tabs();could have been this:
// Insert the tabs into the DOM
tabs.appendChild(frag);
wrapper.insertBefore(tabs, get_single_by_class('js-panel'));
// Listen for clicks on the tab list
add_event(tabs, 'click', clicked);
// Listen for key presses
add_event(tabs, 'keydown', kbd);
// If a tab id is in localStorage open the corresponding panel
if(storage && localStorage['tab']) {
show_hide(localStorage['tab']);
}
};
// Make all that happen
tabs();-
I am pretty sure this
typeof event.target !== 'undefined' ?
x = event.target :
x = event.srcElement;could be
x = event.target || event.srcElement;-
The bigger picture is that this:
/* When a tab has been clicked
========================================================================== */
var clicked = function(event) {
var x,
x_id;
typeof event.target !== 'undefined' ?
x = event.target :
x = event.srcElement;
if(x.nodeName.toLowerCase() === 'li') {
// get the id of the clicked tab
x_id = x.id;
} else {
return; // stop clicks on the hiding everything
}
show_hide(x_id);
};could be
// When a tab has been clicked
var onClicked = function(event) {
var clickedElement = event.target || event.srcElement;
if(clickedElement.nodeName.toLowerCase() === 'li') {
show_hide(clickedElement.id);
}
};- You are not following lowerCamelCase
- You should used named constants for the keycodes
key_code === LEFT_ARROWreads better thankey_code === 37
On the whole, I think you should review your code to reduce the line count. I am not saying that you should play CodeGolf, but this could be written in a fewer lines while keeping the same level of quality.
Code Snippets
/* Insert the tabs into the DOM
========================================================================== */
tabs.appendChild(frag);
wrapper.insertBefore(tabs, get_single_by_class('js-panel'));
/* Listen for clicks on the tab list
========================================================================== */
add_event(tabs, 'click', clicked);
/* Listen for key presses
========================================================================== */
add_event(tabs, 'keydown', kbd);
/* If a tab id is in localStorage open the corresponding panel
========================================================================== */
if(storage && localStorage['tab']) {
show_hide(localStorage['tab']);
}
};
// Make all that happen
tabs();// Insert the tabs into the DOM
tabs.appendChild(frag);
wrapper.insertBefore(tabs, get_single_by_class('js-panel'));
// Listen for clicks on the tab list
add_event(tabs, 'click', clicked);
// Listen for key presses
add_event(tabs, 'keydown', kbd);
// If a tab id is in localStorage open the corresponding panel
if(storage && localStorage['tab']) {
show_hide(localStorage['tab']);
}
};
// Make all that happen
tabs();typeof event.target !== 'undefined' ?
x = event.target :
x = event.srcElement;x = event.target || event.srcElement;/* When a tab has been clicked
========================================================================== */
var clicked = function(event) {
var x,
x_id;
typeof event.target !== 'undefined' ?
x = event.target :
x = event.srcElement;
if(x.nodeName.toLowerCase() === 'li') {
// get the id of the clicked tab
x_id = x.id;
} else {
return; // stop clicks on the <ul> hiding everything
}
show_hide(x_id);
};Context
StackExchange Code Review Q#46727, answer score: 2
Revisions (0)
No revisions yet.