Updating CSS for "active" tab

Question

I'd like to get some feedback on this very basic JavaScript to update which tab is showing as "active" on my simple web page. This is all in a file titled adjustactive.js and included in my footer template for all pages like so:

<script src="assets/js/adjustactive.js"></script>

Here's the full content of that .js file:

//function courtesy of comments in http://befused.com/javascript/get-filename-url
function getFileName() {
    var url = document.location.href;
    url = url.substring(0, (url.indexOf("#") == -1) ? url.length : url.indexOf("#"));
    url = url.substring(0, (url.indexOf("?") == -1) ? url.length : url.indexOf("?"));
    url = url.substring(url.lastIndexOf("/") + 1, url.length);
    if(url.length == 0) url = "index";
    return url;
}


function updateNav() {
    var idName = getFileName();
    document.getElementById(idName).className = "active";
}

window.onload = updateNav();

Have you considered using jQuery? The code would be simpler/cleaner if you used it. — Franz Payer
– Franz Payer, Commented Jul 29, 2015 at 14:37
@FranzPayer I've used it in the past, but I try to avoid it when it doesn't offer a big improvement. This task is simple enough I don't know that it's worth the jquery library download, which I'm otherwise avoiding on this project. — sunny
– sunny, Commented Jul 29, 2015 at 14:42
@FranzPayer, jQuery is not always necessary, you might not need jQuery, and often it's a case of a few characters difference. — Quill
– Quill, Commented Jul 29, 2015 at 14:46
Especially in this case jQuery does nothing easier or faster. — RoToRa
– RoToRa, Commented Jul 29, 2015 at 20:01

RoToRa · Accepted Answer · 2015-07-29 20:14:58Z

First off, you have a bug. This instruction

window.onload = updateNav();

doesn't execute updateNav on load. It executes updateNav immediately and assigns its return value (which is undefined) to onload. Instead it should be:

window.onload = updateNav;

Also assigning event handlers directly to on... properties isn't something you should be doing. The main problem is you are potentially overwriting any other scripts that also addign to onload. You should instead use addEventListener.

You can simplify parsing the URL. document.location already is provides the parts of the URL. In your case document.location.pathname contains the path without domain, query, hash, etc:

function getFileName() {
    var url = document.location.pathname;
    url = url.substring(url.lastIndexOf("/") + 1, url.length);
    if (url.length == 0) url = "index";
    return url;
}

In my opinion you are misusing the id attribute here. This is not what it's made for. (However to be honest I have difficulties finding the right words to explain why not - it just feels wrong.)

Finally you should be aware that this is a bad usage of JavaScript. You are using JS here for something that should be done server-side (that is directly in the HTML).

I agree with/learned from everything you said except the last two points. I'm not sure why id feels wrong, but alright. To the last point, I deliberately moved this out of the HTML because I'd like all files to share the same navigational header (where this css property is being set). Surely it doesn't make sense to set the css hard-coded into the HTML of each page? — sunny
– sunny, Commented Jul 29, 2015 at 20:18
Of course every page should have it's own header with the class set, but that doesn't mean they shouldn't also all share the same header code. Nowadays you usually generate HTML using a template engine, some server-side script or at least a build tool, so you can use that to insert the class at the right position in the header. — RoToRa
– RoToRa, Commented Jul 30, 2015 at 13:54

Stack Exchange Network

Updating CSS for "active" tab

1 Answer 1

You must log in to answer this question.

Hot Network Questions

Updating CSS for "active" tab

1 Answer 1

You must log in to answer this question.

Related

Hot Network Questions