I am wondering how can this code be improved? I have 3 menu buttons in navigation. The first one is active by default. On click, other buttons are assigned with class active.
<div class="mobile-nav">
<ul>
<li><input class="mobile-nav-tab active" id="search-tab" type="button"/></li>
<li><input class="mobile-nav-tab" id="menu-tab" type="button"/></li>
<li><input class="mobile-nav-tab" id="members-tab" type="button"/></li>
</ul>
</div>
each of these buttons will show a certain div and hide the others.
<div class="mobile-nav-content">
<div id="search">
</div>
<div style="display:none" id="menu">
<br>menu
<br>menu
</div>
<div style="display:none" id="members">
<br>members
<br>members
</div>
</div>
I have this code and it is working fine.
$(document).ready(function(){
$('.mobile-nav-tab').click(function(){
$('.mobile-nav-tab').removeClass('active');
$(this).addClass('active');
if ($('#search-tab').hasClass('active')){
$('#search').slideDown();
}
else{
$('#search').hide();
}
if ($('#menu-tab').hasClass('active')){
$('#menu').slideDown();
}
else{
$('#menu').hide();
}
if ($('#members-tab').hasClass('active')){
$('#members').slideDown();
}
else{
$('#members').hide();
}
});
});
I found out that i can shorthand if-else parts with Ternary operator. But i wonder is there any other way to improve it and make it shorter?
I am wondering how can this code be improved? I have 3 menu buttons in navigation. The first one is active by default. On click, other buttons are assigned with class active.
<div class="mobile-nav">
<ul>
<li><input class="mobile-nav-tab active" id="search-tab" type="button"/></li>
<li><input class="mobile-nav-tab" id="menu-tab" type="button"/></li>
<li><input class="mobile-nav-tab" id="members-tab" type="button"/></li>
</ul>
</div>
each of these buttons will show a certain div and hide the others.
<div class="mobile-nav-content">
<div id="search">
</div>
<div style="display:none" id="menu">
<br>menu
<br>menu
</div>
<div style="display:none" id="members">
<br>members
<br>members
</div>
</div>
I have this code and it is working fine.
$(document).ready(function(){
$('.mobile-nav-tab').click(function(){
$('.mobile-nav-tab').removeClass('active');
$(this).addClass('active');
if ($('#search-tab').hasClass('active')){
$('#search').slideDown();
}
else{
$('#search').hide();
}
if ($('#menu-tab').hasClass('active')){
$('#menu').slideDown();
}
else{
$('#menu').hide();
}
if ($('#members-tab').hasClass('active')){
$('#members').slideDown();
}
else{
$('#members').hide();
}
});
});
I found out that i can shorthand if-else parts with Ternary operator. But i wonder is there any other way to improve it and make it shorter?
Share Improve this question asked Apr 8, 2013 at 17:03 DušanDušan 4982 gold badges9 silver badges23 bronze badges5 Answers
Reset to default 4This should shorten it up considerably:
$(".mobile-nav-tab").click(function() {
var parts = this.id.split("-");
$(".mobile-nav-content").children().slideUp();
$('.mobile-nav-tab').removeClass('active');
$(this).addClass("active");
$("#" + parts[0]).slideDown();
});
Simply loop over the various titles:
$(document).ready(function(){
$('.mobile-nav-tab').click(function(){
$('.mobile-nav-tab').removeClass('active');
$(this).addClass('active');
var a=["search","menu","members"]
for (i in a){
if ($('#'+a[i]+'-tab').hasClass('active')){
$('#'+a[i]).slideDown();
}
else{
$('#'+ a[i]).hide();
}
}
});
});
Alternatively, do this:
$(document).ready(function(){
$('.mobile-nav-tab').click(function(){
$('.mobile-nav-tab').removeClass('active');
$(this).addClass('active');
$('.mobile-nav-content div').hide() //Hide all content divs
$('#'+this.id.split('-')[0]).slideDown() //SlideDown the current div only
});
});
I think you can clean it up to this:
$('.mobile-nav-tab').click(function(){
$('.mobile-nav-tab').removeClass('active');
$(this).addClass('active');
var $activeContent = $('#' + $(this).attr('id').replace('-tab',''));
$('.mobile-nav-tab>div').not($activeContent).hide();
$activeContent.slideDown();
});
$('.mobile-nav-tab').on("click", function () {
clickedId = $(this).attr("id");
$('.mobile-nav-tab').each(function (index) {
if ($(this).attr("id") == clickedId) {
$('.mobile-nav-tab,.content-item').eq(index).addClass('active');
$(".content-item").eq(index).slideDown();
} else {
$('.mobile-nav-tab,.content-item').eq(index).removeClass('active');
$(".content-item").eq(index).hide();
}
});
});
http://jsfiddle/megarameno/L9Uge/4/
A) You should start caching objects into variables, such as
var tabs = $('.mobile-nav-tab'),
search = $('#search'),
searchTab = $('#search-tab');
instead of jumping into the DOM again and again
B) There is a jQuery method siblings() that would work like a charm here. You might need to modify your HTML a little bit to get the best result, but it's much easier than selecting each tab separately. It can look something like this:
selectedTab.slideDown().siblings().hide();
which slides down the desired tab and hides all other tabs on the same level in DOM.
I hope this helps you and puts you in some new direction.