patternjavascriptMinor

Tabs in JavaScript and CSS without additional framework/library

Submitted by: @import:stackexchange-codereview·Mar 10, 2026·

Viewed 0 times

withoutcssjavascriptandlibrarytabsframeworkadditional

Problem

Below's my attempt at creating a simple tab control in JavaScript and CSS, without making use of any libraries or frameworks.

Does this look OK to you guys? Can you see any obvious pitfalls? I believe this should work on all modern browsers, supporting as far back as IE9.

jsFiddle

```

Tabs Demo

.tabs {
margin: 10px;
padding: 10px;
}
.tabs * {
margin: 0px;
padding: 0px;
}
.tabs > ul > li {
display: inline;
background-color: lightGrey;
border-bottom-style: solid;
border-bottom-color: white;
border-width: 1px;
-webkit-border-top-right-radius: 10px;
-moz-border-radius-topright: 10px;
border-top-right-radius: 10px;
}
.tabs > ul > li.selected {
display: inline;
border-bottom-color: lightGrey;
}
.tabs > ul > li:first-of-type {
-webkit-border-top-left-radius: 1em;
-moz-border-radius-topleft: 1em;
border-top-left-radius: 1em;
}
.tabs > ul > li > a {
padding: 1em;
text-decoration: none;
}
.tabs > div {
display: none;
}
.tabs > div.selected {
background-color: lightGrey;
display: block;
padding: 1em;
-webkit-border-radius: 2em;
-moz-border-radius: 2em;
border-radius: 2em;
-webkit-border-top-left-radius: 0px;
-moz-border-radius-topleft: 0px;
border-top-left-radius: 0px;
box-shadow: 10px 10px 5px #888888;
}

One
Two
Three
Four
Five
Six

Sed ut perspiciatis unde omnis iste natus error sit voluptatem accusantium doloremque laudantium, totam rem aperiam, eaque ipsa quae ab

Solution

-
The comment start index at 1; not 0 is not helpful; that part of the code is pretty self-explanatory. Rather, you could comment on your reasoning for doing so.

-
The spacing in indexOfElem is horrendous. Please space out your braces properly. If they enclose only one line, you lose the protection against adding code that you think is in the loop, but in reality is not.

-
The use of a while loop in indexOfElem is alright, but you may find the for loop variant more natural. You'll notice that we no longer need to assign index to 1, since our loop (which admittedly uses an additional iteration) will take care of it for us.

function indexOfElem(elem) {
    var index = 0;
    for (var target = elem.nodeType; elem; elem = elem.previousSibling) {
        if (elem.nodeType === target) {
            index++;
        }
    }
    return index;
}

-
The if (no.parentNode) block in selectTab works, but it's kind of icky. It isn't immediately clear that when called with a DOM element, the DOM element will be a direct child of the tab itself. Rather, you can do a more natural check for typeof no, and set the onclick for the ` to selectTab(this.parentNode)

.

- 
In

selectTab, I'd prefer to write the assignment in the if condition. I'd also rather group together the select and deselect code, rather than grouping together the li and div

 code.

- 
Right now, if I

selectTab(-1)

, my tab will be unselected. Instead, it seems like it would be better to check if the index is in bounds first. This is a behavioural change; it's up to you whether you keep it.

- 
You use

elem in indexOfElem, so why use e here? For consistency, rename e to elem. I also renamed no to index

.

function selectTab(index) {
    if (typeof index !== "number") {
        index = indexOfElem(index);
    }
    var max = Math.min(
        document.querySelectorAll(".tabs > div").length,
        document.querySelectorAll(".tabs > ul > li").length
    );
    if (index  max) {
        return;
    }

    // deselect the currently selected tab, if applicable
    // this code could be moved into a separate removeClass function, but it's only used twice here so it's not *that* bad...
    var elem;
    if (elem = document.querySelector(".tabs > ul > li.selected")) {
        elem.classList.remove("selected");
    }
    if (elem = document.querySelector(".tabs > div.selected")) {
        elem.classList.remove("selected");
    }

    // select the new tab. no checking required since we already did a range check
    document.querySelector(".tabs > ul > li:nth-of-type(" + index + ")").classList.add("selected");
    document.querySelector(".tabs > div:nth-of-type(" + index + ")").classList.add("selected");
}


- 
In

windowOnloadPush, you should return early if newOnload isn't executable. As a matter of style I prefer x && x()

, but that's your choice.

function windowOnloadPush(newOnload) {
    if (typeof newOnload !== "function") {
        return;
    }

    var currentOnload = window.onload;
    window.onload = function() {
        currentOnload && currentOnload();
        newOnload();
    };
}


- 
In

loadTabs, if you make the first change to selectTab that I mentioned, you'll need to change to onclick.value = "selectTab(this.parentNode);";

.

- 
Right now you're reassigning all the children of the tab to the

. That's fine, but if you're just trying to set the text, you can use textContent

 instead. Again, this is a behavioural change.

function loadTabs() {
    var elems = document.querySelectorAll(".tabs > ul > li");
    for (var i = 0; i < elems.length; i++) {
        var a = document.createElement('a');
        var href = document.createAttribute('href');
        href.value = "#";
        var onclick = document.createAttribute('onclick');
        onclick.value = "selectTab(this.parentNode);";

        a.setAttributeNode(href);
        a.setAttributeNode(onclick);

        a.textContent = elems[i].textContent;
        elems[i].textContent = "";

        elems[i].appendChild(a);
    }
    selectTab(1);
}


- 
You don't need to use

function() { loadTabs() }; just use loadTabs

:

windowOnloadPush(loadTabs);


- 
Minor stylistic point about your CSS: you can use

0 instead of 0px or #888 instead of #888888

. These aren't big deals though, and it's perfectly fine to leave them as-is.

- 
You've got

padding: 1em on a

 in your CSS. This means that there's some extra click space for each tab — clicking on the top of the content changes the tab, which is unintuitive. Instead, use

padding: 0 1em;. (If you like units, you can use padding: 0em 1em;. DON'T use padding: 0px 1em;

 because, well, why would you?)

- 
You're duplicating

display: inline; on .tabs > ul > li.selected

. You can safely remove this.

- 
Rather than setting

display: none; on .tabs > div and display: block on .tabs > div.selected, you can just set display: none; on .tabs > div:not

Code Snippets

function indexOfElem(elem) {
    var index = 0;
    for (var target = elem.nodeType; elem; elem = elem.previousSibling) {
        if (elem.nodeType === target) {
            index++;
        }
    }
    return index;
}

function selectTab(index) {
    if (typeof index !== "number") {
        index = indexOfElem(index);
    }
    var max = Math.min(
        document.querySelectorAll(".tabs > div").length,
        document.querySelectorAll(".tabs > ul > li").length
    );
    if (index < 1 || index > max) {
        return;
    }

    // deselect the currently selected tab, if applicable
    // this code could be moved into a separate removeClass function, but it's only used twice here so it's not *that* bad...
    var elem;
    if (elem = document.querySelector(".tabs > ul > li.selected")) {
        elem.classList.remove("selected");
    }
    if (elem = document.querySelector(".tabs > div.selected")) {
        elem.classList.remove("selected");
    }

    // select the new tab. no checking required since we already did a range check
    document.querySelector(".tabs > ul > li:nth-of-type(" + index + ")").classList.add("selected");
    document.querySelector(".tabs > div:nth-of-type(" + index + ")").classList.add("selected");
}

function windowOnloadPush(newOnload) {
    if (typeof newOnload !== "function") {
        return;
    }

    var currentOnload = window.onload;
    window.onload = function() {
        currentOnload && currentOnload();
        newOnload();
    };
}

function loadTabs() {
    var elems = document.querySelectorAll(".tabs > ul > li");
    for (var i = 0; i < elems.length; i++) {
        var a = document.createElement('a');
        var href = document.createAttribute('href');
        href.value = "#";
        var onclick = document.createAttribute('onclick');
        onclick.value = "selectTab(this.parentNode);";

        a.setAttributeNode(href);
        a.setAttributeNode(onclick);

        a.textContent = elems[i].textContent;
        elems[i].textContent = "";

        elems[i].appendChild(a);
    }
    selectTab(1);
}

windowOnloadPush(loadTabs);

Context

StackExchange Code Review Q#61074, answer score: 5

Revisions (0)

No revisions yet.