10
\$\begingroup\$

What I'm doing?

I'm creating a member-list where initially only the names are visible. Clicking the names reveals the member information. This is done with jQuery by adding/removing classes.

I left out the CSS with the transitions for this review.

HTML:

<ul class="block-list member-list">
    <li>
        <a href="#" class="block-list__link">member name</a>
        <div class="block-list__content">
            <!-- member information -->
        </div>
    </li>
    <li>
        <a href="#" class="block-list__link">member name</a>
        <div class="block-list__content">
            <!-- member information -->
        </div>
    </li>
    <li>
        <a href="#" class="block-list__link">member name</a>
        <div class="block-list__content">
            <!-- member information -->
        </div>
    </li>
</ul>

jQuery:

<script src="http://code.jquery.com/jquery-1.10.2.min.js"></script>
<script>if (!window.jQuery) { document.write('<script src="/jquery/jquery-1.10.2.min.js"><\/script>'); }</script>

<script type="text/javascript">
    $(document).ready(function(){
        var blContent = $(".block-list__content");

        // If JS is disabled, the member information is visible by default
        $(blContent).addClass("hidden");

        $(".block-list__link").click(function(e) {
            // Prevent fragment identifier being added to the URL
            e.preventDefault();

            // Save the `block-list__content` which is a sibling of the clicked `block-list__link`
            var blCurrentContent = $(this).siblings(".block-list__content");
            if ($(blCurrentContent).hasClass("hidden")) {
                // Remove `visible` class from all `block-list__content`'s and add `hidden` class
                $(blContent).removeClass("visible").addClass("hidden");

                // Remove the `hidden` class from the current `block-list__content` and add `visible` class
                $(blCurrentContent).removeClass("hidden").addClass("visible");
            } else {
                // If `block-list__link` is clicked again, add `hidden` class
                $(blCurrentContent).removeClass("visible").addClass("hidden");
            }
        });
    });
</script>
\$\endgroup\$
5
\$\begingroup\$

I massively don't like your class name convention, but I guess that's personal preference. I understand that you're using the __ to denote subcomponents, but if you follow the principles below then that shouldn't be necessary. Just change your content to .content and your links to .link.

  • Something that your code does that you might not be aware of is bind a click event to each .block-list__link individually. While this is not a big issue when there are only three links, it's better practice to use event delegation:

    $('.block-list').on('click', '.link', function () {
    
    });
    

    This will only bind the event once (to the parent .block-list), but it will only fire when one of the delegates (.link) is clicked.

  • You should never be adding a class onload, that doesn't make sense and may cause flickering as the DOM loads before the JavaScript. Have the class added already in your HTML or better still, set the elements default in your CSS. If the .content elements are invisible by default then you need only have one add/remove class in your JavaScript. Let's use .visible for this.

  • Regarding the hiding/showing, what elclanrs posted is valid, but I still don't think it's very easy to read. I'd prefer something like this, remember the .content blocks are invisible by default;

    var $blockList = $('.block-list'),
      $content = $('.content', $blockList);
    
    $blockList.on('click', '.link', function () {
       // hide the visible one
       var $justHidden = $content.filter('.visible')
         .removeClass('visible');
       // show the closest one, unless it's the one we just hid
       $(this).next('.content').not($justHidden)
         .addClass('visible');
    });
    
\$\endgroup\$
6
\$\begingroup\$

You're double-wrapping elements in jQuery. blContent and blCurrentContent are already jQuery objects, so doing $(blContent) isn't necessary. It's common practice to prefix jQuery collections with a dollar sign $.

Other than that your code looks fine, but you could use toggleClass and exclude the current sibling element from the previous collection:

var $blContent = $('.block-list__content').addClass('hidden');

$('.block-list__link').click(function(e) {
  e.preventDefault();
  var $content = $(this)
    .siblings('.block-list__content')
    .toggleClass('hidden visible');
  $blContent
    .not($content)
    .removeClass('visible')
    .addClass('hidden');
});
\$\endgroup\$
5
  • \$\begingroup\$ I'm a jQuery beginner. Wouldn't clicking a link for an already expanded div 1.) Remove visible and add hidden and then 2.) toggle it, which will remove hidden again and add visible. Therefor I won't be able to hide the divs anymore, right? \$\endgroup\$ Feb 8 '14 at 9:25
  • \$\begingroup\$ It works the same as your code, I'm not sure I understand the question... \$\endgroup\$
    – elclanrs
    Feb 8 '14 at 9:29
  • \$\begingroup\$ No, it doesn't. Clicking a block-list__link with an already visible block-list__content won't hide it again. I believe it adds hidden while removing all visible classes and right away toggles it back to visible. \$\endgroup\$ Feb 8 '14 at 9:31
  • \$\begingroup\$ Oh I didn't notice that. Well it's because the line above add the class, you'd have to exclude the current sibling, see if this works as expected jsbin.com/geboq/2/edit and I'll update the answer \$\endgroup\$
    – elclanrs
    Feb 8 '14 at 9:37
  • \$\begingroup\$ I updated the question with slightly better code, that should do. \$\endgroup\$
    – elclanrs
    Feb 8 '14 at 9:42

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.