2
\$\begingroup\$

Activating jQuery tabs from URL #id is www.somedomain.com/index.php#tab3

Given this my best shot, and it does work but I feel it could be far far better.

Please tell me if this is totally shockingly bad JS.

$(document).ready(function() {
//When page loads...
$(".tab_content").hide(); //Hide all content
    //On Click Event
$("ul.tabs li").click(function (e) {
    e.preventDefault();
    $("ul.tabs li").removeClass("active"); //Remove any "active" class
    $(this).addClass("active"); //Add "active" class to selected tab
    $(".tab_content").hide(); //Hide all tab content
    var activeTab = $(this).find("a").attr("href"); //Find the href attribute value to identify the active tab + content
    $(activeTab).fadeIn(); //Fade in the active ID content
});
if(document.location.hash!='') {
    //get the index from URL hash
    var tabName = window.location.hash;
    $(tabName).show(); //Show tab from url hash
    var tabNumber = tabName.substr(4,1); //shorten #tab2 to just last digit "2"
    $("ul.tabs li:nth-child(tabNumber)").addClass("active").show();    
} else {

    $("ul.tabs li:first").addClass("active").show(); //Activate first tab
    $(".tab_content:first").show(); //Show first tab content
}

});

Thank you

\$\endgroup\$
  • \$\begingroup\$ Have you considered using jQuery BBQ? \$\endgroup\$ – Philip Seyfi Nov 18 '11 at 17:22
  • \$\begingroup\$ $("ul.tabs li:nth-child(tabNumber)") won't work as tabNumber is treated as literal string in the selector. \$\endgroup\$ – Tushar Aug 16 '17 at 12:09
2
\$\begingroup\$

Well, you could do some refactoring with this code, the only issue I see is that the low level functinality is implemented at the top level of your code. It would be better to extract the logic to separate functions, something like this (I also improved your code formatting a little bit):

$((function() { // Deleted the "document).ready" part, that is not necessary.
    // Hide all content.
    $(".tab_content").hide();

    // If hash is present in the URL, show tab by hash. Otherwise, show the first tab.
    if(document.location.hash!='') {
        showTabByHash();
    }
    else {
        showFirstTab()
    }

    // On the click event of a tab.
    $("ul.tabs li").click(function (e) {
        e.preventDefault();
        showTab($this);
    });
});

function showTab(tab) {
    // Remove any "active" class.
    $("ul.tabs li").removeClass("active");

    // Add "active" class to selected tab.
    tab.addClass("active");

    // Hide all tab content
    $(".tab_content").hide();

    // Find the href attribute value to identify the active tab + content
    var activeTab = tab.find("a").attr("href");

    // Fade in the active ID content.
    $(activeTab).fadeIn();
}

function showTabByHash() {
    // Get the index from URL hash.
    var tabName = window.location.hash;
    // Shorten #tab2 to just last digit "2".
    var tabNumber = tabName.substr(4,1);

    // Why are these two lines needed? Don't they work with the same tab element?
    $(tabName).show(); //Show tab from url hash
    $("ul.tabs li:nth-child(tabNumber)").addClass("active").show();
}

function showFirstTab() {
    // Activate first tab.
    $("ul.tabs li:first").addClass("active").show();

    // Show first tab content.
    $(".tab_content:first").show();
}

I do not see any other problems with the code (of course, it could be generalized a little bit, but that is not your goal if I understand right), apart from your coding style, which was a little bit cluttered in my opinion (I had to read it more than one time to completely understand it). And if you are developing in Visual Studio, and these functions are going to be used from somewhere else, then adding XML comments to them would generally be a good idea.

|improve this answer|||||
\$\endgroup\$
  • \$\begingroup\$ Thank you Mark. That's really helpful. I'm using Notepad++ and attempting to get my JS knowledge up to scratch. Thanks again. \$\endgroup\$ – user1001751 Oct 18 '11 at 18:30
  • \$\begingroup\$ You're welcome. However, I am not a JS guru either, so maybe some JS guys will come and find something else there, who knows :). \$\endgroup\$ – Mark Oct 18 '11 at 18:39
  • \$\begingroup\$ Old question, wouldn't it be better to trigger the tag with a click event? That way if you have any functions that get called when the tab is clicked, they would also be executed. \$\endgroup\$ – vol7ron Aug 29 '13 at 1:33

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy