Problem
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
Solution
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.