Tabs in JavaScript and CSS without additional framework/library

Question

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

<!DOCTYPE html>
<html>
<head>
    <title>Tabs Demo</title>
    <meta http-equiv="Content-Type" content="text/html;charset=UTF-8">
    <style type="text/css">
        .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;
        }
    </style>
</head>
<body lang="en-US">
    <div class="tabs">
        <ul>
            <li>One</li>
            <li>Two</li>
            <li>Three</li>
            <li>Four</li>
            <li>Five</li>
            <li>Six</li>
        </ul>
        <div>Sed ut perspiciatis unde omnis iste natus error sit voluptatem accusantium doloremque laudantium, totam rem aperiam, eaque ipsa quae ab illo inventore veritatis et quasi architecto beatae vitae dicta sunt explicabo. Nemo enim ipsam voluptatem quia voluptas sit aspernatur aut odit aut fugit, sed quia consequuntur magni dolores eos qui ratione voluptatem sequi nesciunt. Neque porro quisquam est, qui dolorem ipsum quia dolor sit amet, consectetur, adipisci velit, sed quia non numquam eius modi tempora incidunt ut labore et dolore magnam aliquam quaerat voluptatem. Ut enim ad minima veniam, quis nostrum exercitationem ullam corporis suscipit laboriosam, nisi ut aliquid ex ea commodi consequatur? Quis autem vel eum iure reprehenderit qui in ea voluptate velit esse quam nihil molestiae consequatur, vel illum qui dolorem eum fugiat quo voluptas nulla pariatur?</div>
        <div>2 - two</div>
        <div>3 - three</div>
        <div>Lorem ipsum dolor sit amet, consectetur adipiscing elit, sed do eiusmod tempor incididunt ut labore et dolore magna aliqua. Ut enim ad minim veniam, quis nostrud exercitation ullamco laboris nisi ut aliquip ex ea commodo consequat. Duis aute irure dolor in reprehenderit in voluptate velit esse cillum dolore eu fugiat nulla pariatur. Excepteur sint occaecat cupidatat non proident, sunt in culpa qui officia deserunt mollit anim id est laborum</div>
        <div>5</div>
        <div>6</div>
    </div>
    <script type="text/javascript">

        function indexOfElem(elem){
            var  i = 1; //start index at 1; not 0
            //this.parentNode.querySelectorAll('li:preceding-sibling').length
            //https://developer.mozilla.org/en-US/docs/Web/API/Node.nodeType
            var desiredNodeType = elem.nodeType;
            while((elem=elem.previousSibling)!=null) {if(elem.nodeType===desiredNodeType)i++;}
            return i;
        }
        function selectTab(no) {
            if(no.parentNode) {
                no = indexOfElem(no.parentNode);
            }
            //update the tab's link
            var e = document.querySelector(".tabs > ul > li.selected");
            if(e) e.classList.remove("selected");
            e = document.querySelector(".tabs > ul > li:nth-of-type(" + no + ")")
            if(e) e.classList.add("selected");
            //update the tab (div)
            e = document.querySelector(".tabs > div.selected");
            if(e) e.classList.remove("selected");
            e = document.querySelector(".tabs > div:nth-of-type(" + no + ")");
            if(e) e.classList.add("selected");
        }
        function windowOnloadPush(newOnload) {
            var currentOnload = window.onload;
            window.onload = function () {
                if (currentOnload) {
                    currentOnload();
                }
                if (newOnload) {
                    newOnload();
                }
            };
        }
        function loadTabs() {
            var es = document.querySelectorAll(".tabs > ul > li");
            for (var i=0; i < es.length; i++) {
                var a = document.createElement('a');
                var href = document.createAttribute('href');
                href.value = "#";
                var onclick = document.createAttribute('onclick');
                onclick.value = "selectTab(this);";
                a.setAttributeNode(href);
                a.setAttributeNode(onclick);
                while (es[i].childNodes.length > 0) {
                    a.appendChild(es[i].childNodes[0]);
                }
                es[i].appendChild(a);
            }
            selectTab(1);
        }
        windowOnloadPush(function () { loadTabs(); });
    </script>
</body>
</html>

Don't edit your code after posting it. It's difficult to critic a moving target. And the fiddle is no longer identical to the code you posted. Also: The fiddle doesn't work because you set the script to be executed "onload". Change it to "No wrap - in <body>". — RoToRa, Aug 26 '14 at 13:37
@RoToRa; apologies, slipped in a tweak to limit the amount of HTML which needed to be added to get it to work in unison with the JS, instead having the JS create what it could automatically. Thanks to Schism for updating per your comments. — JohnLBevan, Aug 26 '14 at 14:27

Schism · Accepted Answer · 2014-08-26 15:04:19Z

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 <a> 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 < 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");
}

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 <a>. 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(.selected). It's up to you to decide which reads better, though I should note that this approach is probably ever so slightly less performant.
It would be nice if the <ul> was auto-generated. You could set a data-name or data-title or something on each <div> and then create the <ul> in loadTabs(). I didn't implement this, but you should think about it.

Here's a fiddle with my revised code.

Great comments, really comprehensive; thanks @Schism. – JohnLBevan Aug 26 '14 at 15:37 — JohnLBevan, Aug 26 '14 at 15:37

Si Kelly · Answer 2 · 2014-08-26 16:11:58Z

A couple of points:

You don't need the function windowOnloadPush at all in fact in modern browsers you can use:
```
window.addEventListener("load", loadTabs);
```

You don't need to add your onclick handler as a String (I guess you are doing this for a correct version of this). Instead you can use an inline function that you execute immediately to which you pass the element and index. Or you can split this out to its own function. You also don't need to add the attributes explicitly to the <a> tag:

  function loadTabs() {
      var es = document.querySelectorAll(".tabs > ul > li");
      for (var i = 0; i < es.length; i++) {
          (function (el,idx) {
              var a = document.createElement('a');
              a.href= "#";
              a.onclick = function () {
                  selectTab(idx);
              };
              while (el.childNodes.length > 0) {
                  a.appendChild(el.childNodes[0]);
              }
              el.appendChild(a);
          })(es[i],i+1);
      }
      selectTab(1);
  }

Here's my altered fiddle:

http://jsfiddle.net/dc5v6f6n/2/

One more point:

Don't set href="#", set it to "javascript:void(0);". This is because linking to # has the side-effect of scrolling to the top of the page. Not good if your tabs are not at the very top and the user has scrolled down a bit.

Thanks @SiKelly. I'd not come across that approach with inline functions before; nice trick. — JohnLBevan, Aug 26 '14 at 16:04

asked	1 year ago
viewed	707 times
active	1 year ago

current community

your communities

more stack exchange communities

Tabs in JavaScript and CSS without additional framework/library

2 Answers 2

Your Answer

Not the answer you're looking for? Browse other questions tagged javascript css gui or ask your own question.

Hot Network Questions

current community

your communities

more stack exchange communities

Tabs in JavaScript and CSS without additional framework/library

2 Answers 2

Your Answer

Sign up or log in

Post as a guest

Not the answer you're looking for? Browse other questions tagged javascript css gui or ask your own question.

Related

Hot Network Questions