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?

• javascript
• jquery
• html
• css

5 Answers 5

This 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.