Toggling html buttons and text

Question

I went through a question on SO, and optimized the code according to OP's requirements. I would like a review if it is done properly or not.

Here is the code snippet, there are 2 level buttons, when clicked 1st level button, it toggles second level button group.

Second level button group toggles div, which contains checkbox. Any review and suggestions are appreciated.

$(document).ready(function() {

  var groups = ['a', 'b', 'c']; // here

  $.each(groups, function(k, id) {
    $('#' + id).hide();
    $('#' + id + 'l').hide();
  });

  $(".button").click(function() {

    var button_id = $(this).data('id');

    $('#' + button_id).toggle();

    var hide = $.grep(groups, function(value) {
      return value != button_id;
    });

    $.each(hide, function(k, id) {
      $('#' + id).hide();
    });

  });

  var selector = [];

  $.each(groups, function(k) {
    selector.push('.' + groups[k]);
  });

  selector = selector.join(',');

  $(selector).click(function() {

    var button_id = $(this).data('id');
    var class_id = $(this).attr('class');
    var flag = $('.' + class_id + 'l').filter('[data-id="' + button_id + '"]').is(':visible');

    $.each(groups, function(k, id) {
      $('#' + id + 'l').children().hide();
    });

    $.each(groups, function(k, id) {
      $('#' + id + 'l').hide();
    });

    $('#' + class_id + 'l').show();

    if (flag)
      $('.' + class_id + 'l').filter('[data-id="' + button_id + '"]').hide();
    else
      $('.' + class_id + 'l').filter('[data-id="' + button_id + '"]').show();
  });

});

<script src="https://ajax.googleapis.com/ajax/libs/jquery/2.1.1/jquery.min.js"></script>
<button class="button" data-id="a">Toggle a</button>
<button class="button" data-id="b">Toggle b</button>
<button class="button" data-id="c">Toggle c</button>

<div id="a">
  <div class="a" data-id="1"><button>Toggle a1</button></div>
  <div class="a" data-id="2"><button>Toggle a2</button></div>
  <div class="a" data-id="3"><button>Toggle a3</button></div>
</div>

<div id="b">
  <div class="b" data-id="1"><button>Toggle b1</button></div>
  <div class="b" data-id="2"><button>Toggle b2</button></div>
  <div class="b" data-id="3"><button>Toggle b3</button></div>
</div>

<div id="c">
  <div class="c" data-id="1"><button>Toggle c1</button></div>
  <div class="c" data-id="2"><button>Toggle c2</button></div>
  <div class="c" data-id="3"><button>Toggle c3</button></div>
</div>

<div id="al">
  <div class="al" data-id="1">this is line a1</div>
  <div class="al" data-id="2">this is line a2</div>
  <div class="al" data-id="3">this is line a3</div>
</div>

<div id="bl">
  <div class="bl" data-id="1">this is line b1</div>
  <div class="bl" data-id="2">this is line b2</div>
  <div class="bl" data-id="3">this is line b3</div>
</div>

<div id="cl">
  <div class="cl" data-id="1">this is line c1</div>
  <div class="cl" data-id="2">this is line c2</div>
  <div class="cl" data-id="3">this is line c3</div>
</div>

Answer 1

I recommend an approach where top level buttons, sub-buttons and content are all grouped separately. This way, these grouping wrappers can be used to scope selectors and grab all the top-level buttons into one variable b) all sub-buttons into one variable and c) all content into one variable. These grouping variables can be used to toggle off visibility for all items in that group and then the data-id attributes can be used to toggle on specific items.

Also, I've changed it so the content (lines of text) have unique selectors and the data-id on the sub-buttons has complete selectors for their target content. This way, each sub-button doesn't have to know anything about the top-level button that toggled it.

To keep track of when something is supposed to be turned off and nothing is supposed to be turned on (ie if you click the "Toggle a" button twice in a row), there are state variables for the last chosen top button and last chosen sub-button.

Since this is not in a form, you might want to stick with <a> tags.

Generally, where possible, it's best to save your DOM queries.

Hope this helps.

$(document).ready(function() {

    var $topBtns = $('#top-level button'),
        $subBtns = $('#second-level ul').hide(),
        $content = $('#content p').hide(),
        currentTop,
        currentSecond;

    $topBtns.on('click', function() {
        $content.hide();
        $subBtns.hide();
        if (currentTop === this) {
            currentTop = undefined;
        } else {
            currentTop = this;
            currentSecond = undefined;
            $('#' + this.dataset.id).show();
        }
    });

    $subBtns.find('button').on('click', function() {
        $content.hide();
        if (currentSecond === this) {
            currentSecond = undefined;
        } else {
            currentSecond = this;
            $('#' + this.dataset.id).show();
        }
    });
});

ul {
    padding: 0;
}
.primary-btn {
    list-style: none;
    display: inline;
}

<script src="https://ajax.googleapis.com/ajax/libs/jquery/2.1.1/jquery.min.js"></script>
<ul id="top-level">
    <li class="primary-btn"><button data-id="a">Toggle a</button></li>
    <li class="primary-btn"><button data-id="b">Toggle b</button></li>
    <li class="primary-btn"><button data-id="c">Toggle c</button></li>
</ul>
<div id="second-level">
  <ul id="a">
    <li><button data-id="a1">Toggle a1</button></li>
    <li><button data-id="a2">Toggle a2</button></li>
    <li><button data-id="a3">Toggle a3</button></li>
  </ul>

  <ul id="b">
    <li><button data-id="b1">Toggle b1</button></li>
    <li><button data-id="b2">Toggle b2</button></li>
    <li><button data-id="b3">Toggle b3</button></li>
  </ul>

  <ul id="c">
    <li><button data-id="c1">Toggle c1</button></li>
    <li><button data-id="c2">Toggle c2</button></li>
    <li><button data-id="c3">Toggle c3</button></li>
  </ul>
</div>
<div id="content">
  <div>
    <p id="a1">this is line a1</p>
    <p id="a2">this is line a2</p>
    <p id="a3">this is line a3</p>
  </div>
  <div>
    <p id="b1">this is line b1</p>
    <p id="b2">this is line b2</p>
    <p id="b3">this is line b3</p>
  </div>
  <div>
    <p id="c1">this is line c1</p>
    <p id="c2">this is line c2</p>
    <p id="c3">this is line c3</p>
  </div>
</div>

Answer 2

It's a bad practice to reuse a variable for multiple purposes, especially so when the types are different too: selector is first an array and then becomes a string.

Avoid duplication logic in both branches of a conditional like here:

if (flag)
     $('.' + class_id + 'l').filter('[data-id="' + button_id + '"]').hide();
else
     $('.' + class_id + 'l').filter('[data-id="' + button_id + '"]').show();

Extract the dom selector with the filter, store the result in a variable, and in the branches of the if-else do the hide and show.

It's also recommended to use braces always, even with single statement if-else.

It seems to me that some operations could be joined together and done in a single .each, for example here:

$.each(groups, function(k, id) {
  $('#' + id + 'l').children().hide();
});

$.each(groups, function(k, id) {
  $('#' + id + 'l').hide();
});