1
\$\begingroup\$

I'm new to jQuery, but I have a fair bit of experience with JavaScript, mainly using Angular.

What I'm wanting to do is have implement a horizontal list of tabs, where clicking each button will display a corresponding div, and hide the others.

Here's how I'm doing it:

$(document).ready(function(){

    $(".tab-group").each(function(i, tg){

        $(tg).children().each(function(i, child){
            $(child).click(function(){

                $(tg).children().removeClass('btn-selected');
                $(child).addClass('btn-selected');

                $(tg).children().each(function(i, child) {
                  $('#'+$(child)[0].attributes['tabid'].nodeValue).hide();
                })

                $('#' + $(child)[0].attributes['tabid'].nodeValue).show();
            });
        });

        $(tg).children('.btn-selected').click();

    });

});

See the plunker

This works how I want it to, but I have questions about the code style, because it looks gnarly perhaps:

  • Is there a better way to do ID selectors other than $('#' + ....)
  • What about where I'm always referencing the first time in an array $(child)[0] - that seems messy.
  • Am I reinventing the wheel here? - Is there a better way of doing this?
  • I'm doing a 'hide all the children, then show the clicked one' algorithm. Is there a better way to do this?
\$\endgroup\$
1
\$\begingroup\$

Some thoughts in reviewing both your javascript code and your HTML on Plunker.

Code Style

Don't use spaces between HTML tag properties and their values. It makes your markup hard to read.

Example:

<div id = "footab" class = "module">

Should be:

 <div id="footab" class="module">

Think of the spaces as the ONLY separator between each element property, since in HTML you don't have the luxury of commas. The only usage for spaces in element definition is for this purpose (excepting spaces inside property string values, of course).


Lots of unnecessary vertical white space both in your HTML and javascript code.

Code Content

I question introducing your own custom custom HTML tag properties. This is what data attributes are meant to do. You could change this:

<div id = "foo-btn" tabid = "footab" class = "btn btn-underline btn-selected">FOO</div>

To:

<div id="foo-btn" data-tab-target-id="footab" class="btn btn-underline btn-selected">FOO</div>

So when trying to determine target ID in javascript. You go from this:

$('#'+$(child)[0].attributes['tabid'].nodeValue).hide();

To this:

  var target = this.data('tabTargetId');
  $('#'+target).hide();

Much cleaner. Also note I used this here as...


You are introducing a scope confusion problem by using child instead of just using this.

This:

    $(tg).children().each(function(i, child){

Should just be:

    $(tg).children().each(function(){

Since you are not really using the index value anyway, there is no reason to specify parameters for the callback function. You would then use this or $(this) (if you need jQuery wrapper on current element) from within the callback.


Attach listeners at a higher level (delegate event handling). There is no reason to attach a click handler to every tab in a loop like this. Instead consider using on() with a selector (ideally you could add a class to tab group elements to make this selector simple). So this:

$(".tab-group").each(function(){
    $(this).children().each(function(){

Can simply become:

$(".tab-group").on('click', '.tab-option', function() {

You have removed two levels of nested loops in doing this.


Thinks about whether it truly makes sense to use <h1> tags for title for the tabbed content. Semantically, I would think you should be using <h2> here perhaps. This is more meaningful for things such as Google search indexing.


Putting it together, you may end up with code like this.

HTML:

  <body>
    <h1>Hello Plunker!</h1>
    <div class="tab-group">
        <div id="foo-btn" data-tab-target-id="footab" class="tab-option btn btn-underline btn-selected">FOO</div>
        <div id="bar-btn" data-tab-target-id="bartab" class="tab-option btn btn-underline">BAR</div>
    </div>
    <div id="footab" class="module"> 
      <h2>FOO</h2>
    </div>
    <div id = "bartab" class = "module">
      <h2>BAR</h2>
    </div>

    <div class="tab-group">
        <div data-tab-target-id="a" class="tab-option btn btn-underline btn-selected"> A</div>
        <div data-tab-target-id="b" class="tab-option btn btn-underline"> B</div>
        <div data-tab-target-id="c" class="tab-option btn btn-underline"> C</div>
    </div>
    <div id="a" class="module">
      <h2>ALPHA</h2>
    </div>
    <div id="b" class="module"> 
      <h2>BETA</h2>
    </div>
    <div id="c" class="module"> 
      <h2>CHARLIE</h2>
    </div>
  </body>

Javascript:

$(document).ready(function(){
    $(".tab-group").on('click', '.tab-option', function() { 
       var $selectedTab = $(this);
       var selectedTargetId = $(selectedTab).data('tab-target-id');
       var $siblingTabs = $selectedTab.siblings('.tab-option');

       $siblingTabs
           .removeClass('btn-selected')
           .each(function() {
               var target = $(this).data('tab-target-id');
               $('#'+target).hide();
           });

       $selectedTab.addClass('btn-selected');
       $('#'+selectedTargetId).show();
    });
});

A few thoughts for further exploration:

  • Something like this might be something you consider writing as a jQuery plug-in. In fact there are probably a large number of such plug-ins available around tabbed interfaces that you can either use or reference for inspiration.
  • You could optimize performance here. This code (both your version and mine) would still do some unnecessary DOM traversal that could be designed away if you REALLY needed to squeeze performance out of this functionality. You could certainly better store references between tab group and target content elements such that you would not need to re-calculate these relationships every time this event handler fires. You might take a look at my answer on this code review - https://codereview.stackexchange.com/a/135858/23727 - which presented a more complex use case similar to yours in behavior (controls that show/hide elements). For this use case, I had suggested using a javascript "class" structure to:
    • Support efficient storage of jQuery collection references
    • Build and store a data structure for efficient mapping of control elements to the elements they control
    • Execute various show/hide (filtering) operations.
\$\endgroup\$
  • \$\begingroup\$ I implemented your recommendations as a plunk here - Is there a reason you prefixed the variables with $? Also - it needs $('.tab-group > .btn-selected').click(); at the end to set initial state. \$\endgroup\$ – dwjohnston Sep 19 '16 at 23:07
  • 1
    \$\begingroup\$ @dwjohnston It is relatively common coding style to place $ in front of variable name when the variable represents a jQuery collection result. This can help make things clearer to other developers around when you are working with jQuery wrapper vs. an actual DOM element. Totally a styling choice that you can choose to use or ignore based on you or your team's styling preference. \$\endgroup\$ – Mike Brant Sep 20 '16 at 21:39
1
\$\begingroup\$

You can get rid of loops to set listener. You can just set a selector in a way that it works for all combination. Also it better to navigate to necessary element using this.

 $(".tab-group .btn").on("click", function() {
   var mId = $(this).attr('tabid');
   $("#" + mId).closest('.container').find('.module').hide();
   $("#" + mId).show();
   $(this).parent().find('.btn').removeClass('btn-selected')
   $(this).addClass('btn-selected');
 })

Second, I have wrapped your child elements in a div. Instead of having a long list of divs, you should group them. This creates a tree like structure and make it easy for you to navigate.

<div class="container">
  <div id="footab" class="module">
    <h1> FOO</h1>
  </div>
  <div id="bartab" class="module">
    <h1> BAR</h1>
  </div>
</div>

Third, $(document).ready() is an initialization function. It should call necessary code and not have it. You should break your code in smaller functions and then call them in your .ready function. Idea is if any random person is reading your code, he should know what is happening without diving deep in your code. For this, names (variables and functions) are very important.

$(document).ready(function() {
  registerEvents();
  initUI();
});

Updated Plunker

\$\endgroup\$

Your Answer

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

Not the answer you're looking for? Browse other questions tagged or ask your own question.