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

Accessible tabbed UI

Submitted by: @import:stackexchange-codereview··
0
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

Solution

From a once over:

  • 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_ARROW reads better than key_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.