1
\$\begingroup\$

I have a script that creates an active CSS on link, but I think there are a lot of if statements. Can someone help me refactor this?

var link = window.location.pathname;
var currentPageName = window.location.href.match(/[^\/]+$/)[0];

var hl = $('#ih').val();



if (hl == 'True') {
    var currentPageName = 'WVD';
}

if (window.location.href.indexOf("R") > -1) {
    var currentPageName = window.location.href.match(/[^\/]+$/)[0];
}

if (window.location.href.indexOf("C") > -1) {
    var currentPageName = window.location.href.match(/[^\/]+$/)[0];
}

if (location.pathname == "/") {
    var currentPageName = 'MD';
}

$(".main-navigation ul li a").each(function () {
    var link = $(this).attr("href");
    if (link.indexOf(currentPageName) > -1) {
        $(this).addClass("active");
        $(this).css("color", "white");
        $(this).css("text-decoration", "bold");
    }
});
\$\endgroup\$
1
  • \$\begingroup\$ Can you explain how you came up with this? That way we can find other ways to do it. I'm pretty sure setting an active based on the url isn't this long. \$\endgroup\$
    – Joseph
    May 6, 2014 at 8:20

1 Answer 1

1
\$\begingroup\$

Please consider that using Javascript might not be the best solution. If you use something like PHP, passing a variable to the file generating the html setting which one is active is a cleaner solution.

Anyway..

First it seems that the following variable is unused and can be removed:

var link = window.location.pathname;

Then this line:

var currentPageName = window.location.href.match(/[^\/]+$/)[0];

will trigger an error if the url is root (http://www.example.com/) and it's the same as in these ifs, which also can be combined to one:

if (window.location.href.indexOf("R") > -1) {
    var currentPageName = window.location.href.match(/[^\/]+$/)[0];
}

if (window.location.href.indexOf("C") > -1) {
    var currentPageName = window.location.href.match(/[^\/]+$/)[0];
}

which means that one of them is redundant and can be removed.

You can also remove var in all but the first var currentPageName

An easier way to add the active effect would be to use the attribute contains selector like this:

selector = '.main-navigation ul li a[href*="'+currentPageName+'"]"'
$(selector).css({
    'color': 'white',
    'font-weight': 'bold',
}).addClass('active');

This means that the complete code could look like this:

var match = window.location.href.match(/[^\/]+$/);
var currentPageName = '';

if (location.pathname == '/') {
    currentPageName = 'MD';
} else if (match != null) {
    currentPageName = match[0];
}

var hl = $('#ih').val();

if (hl == 'True') {
    currentPageName = 'WVD';
}

selector = '.main-navigation ul li a[href*="'+currentPageName+'"]"'
$(selector).css({
    'color': 'white',
    'font-weight': 'bold',
}).addClass('active');
\$\endgroup\$
1
  • \$\begingroup\$ Txnaks only this one I have to added to maake it work if (window.location.href.indexOf("R") > -1) { var currentPageName = window.location.href.match(/[^\/]+$/)[0]; } if (window.location.href.indexOf("C") > -1) { var currentPageName = window.location.href.match(/[^\/]+$/)[0]; } \$\endgroup\$
    – schneider
    May 7, 2014 at 12:08

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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