Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I've created this fiddle: http://jsfiddle.net/9SXLJ/ to demonstrate something I've been working on. I've been trying to mimic the behaviour of Twitter where a tweet is expanded, the border-radius and margins of the previous and next tweets are altered.

I've used toggleclass to get this working. I would like advice about how to clean up my code. I'm sure I have more lines of code than I need and I'm sure there are more simple or better ways to achieve the desired effect.

Thanks

HTML

<div id="lists">
      <div class="header">
        <h4 style="display: inline-block;">Header</h4>

        <div class="btn-group pull-right">
          <button class="agree btn btn-mini dropdown-toggle" data-toggle="dropdown">Sort by</button>

          <ul class="dropdown-menu">
            <li><a href="#">1</a></li>
            <li><a href="#">2</a></li>
            <li><a href="#">3</a></li>
          </ul>
        </div>
      </div>

      <div class="list">
        <div class="listname">
          <button class="share btn btn-mini pull-left"><i class="icon-heart-logo"></i></button>

          <div class="btn-group pull-right">
            <button class="btn btn-mini dropdown-toggle pull-right" data-toggle="dropdown"><i class="icon-5hearts"></i></button>

            <ul class="dropdown-menu">
              <li><a href="#">1</a></li>
              <li><a href="#">2</a></li>
              <li><a href="#">3</a></li>
            </ul>
          </div>
          <a class="expandfirst btn" href="#">Colors<span class="pull-right" style="margin-right: 8px;">56,789</span></a>
        </div>

        <ol class="things">
          <li><a href="#">White</a><span class="pull-right">56,789</span></li>
          <li><a href="#">Blue</a><span class="pull-right">56,789</span></li>
          <li><a href="#">Red</a><span class="pull-right">56,789</span></li>
          <li><a href="#">Green</a><span class="pull-right">56,789</span></li>
          <li><a href="#">Orange</a><span class="pull-right">56,789</span></li>
        </ol>
      </div>     

      <div class="list">
        <div class="listname">
          <button class="share btn btn-mini pull-left"><i class="icon-heart-logo"></i></button>

          <div class="btn-group pull-right">
            <button class="btn btn-mini dropdown-toggle pull-right" data-toggle="dropdown"><i class="icon-5hearts"></i></button>

            <ul class="dropdown-menu">
              <li><a href="#">1</a></li>
              <li><a href="#">2</a></li>
              <li><a href="#">3</a></li>
            </ul>
          </div>
          <a class="expand btn" href="#">Colors<span class="pull-right" style="margin-right: 8px;">56,789</span></a>
        </div>

        <ol class="things">
          <li><a href="#">White</a><span class="pull-right">56,789</span></li>
          <li><a href="#">Blue</a><span class="pull-right">56,789</span></li>
          <li><a href="#">Red</a><span class="pull-right">56,789</span></li>
          <li><a href="#">Green</a><span class="pull-right">56,789</span></li>
          <li><a href="#">Orange</a><span class="pull-right">56,789</span></li>
        </ol>
      </div>         

      <div class="list">
        <div class="listname">
          <button class="share btn btn-mini pull-left"><i class="icon-heart-logo"></i></button>

          <div class="btn-group pull-right">
            <button class="btn btn-mini dropdown-toggle pull-right" data-toggle="dropdown"><i class="icon-5hearts"></i></button>

            <ul class="dropdown-menu">
              <li><a href="#">1</a></li>
              <li><a href="#">2</a></li>
              <li><a href="#">3</a></li>
            </ul>
          </div>
          <a class="expand btn" href="#">Colors<span class="pull-right" style="margin-right: 8px;">56,789</span></a>
        </div>

        <ol class="things">
          <li><a href="#">White</a><span class="pull-right">56,789</span></li>
          <li><a href="#">Blue</a><span class="pull-right">56,789</span></li>
          <li><a href="#">Red</a><span class="pull-right">56,789</span></li>
          <li><a href="#">Green</a><span class="pull-right">56,789</span></li>
          <li><a href="#">Orange</a><span class="pull-right">56,789</span></li>
        </ol>
      </div>

      <div class="list">
        <div class="listname">
          <button class="share btn btn-mini pull-left"><i class="icon-heart-logo"></i></button>

          <div class="btn-group pull-right">
            <button class="btn btn-mini dropdown-toggle pull-right" data-toggle="dropdown"><i class="icon-5hearts"></i></button>

            <ul class="dropdown-menu">
              <li><a href="#">1</a></li>
              <li><a href="#">2</a></li>
              <li><a href="#">3</a></li>
            </ul>
          </div>
          <a class="expand btn" href="#">Colors<span class="pull-right" style="margin-right: 8px;">56,789</span></a>
        </div>

        <ol class="things">
          <li><a href="#">White</a><span class="pull-right">56,789</span></li>
          <li><a href="#">Blue</a><span class="pull-right">56,789</span></li>
          <li><a href="#">Red</a><span class="pull-right">56,789</span></li>
          <li><a href="#">Green</a><span class="pull-right">56,789</span></li>
          <li><a href="#">Orange</a><span class="pull-right">56,789</span></li>
        </ol>
      </div>  

  </div>

CSS

@import url('http://twitter.github.com/bootstrap/assets/css/bootstrap.css');

#lists {
  padding-top: 20px;
}

.nav > li > a {
  font-weight: bold;
}
.list .btn-group, .list .share {
  margin: 7px;
}

.header {
  background: #fff;
  border: 1px solid #ccc;
  border-radius: 5px 5px 0 0;
  font-weight: bold;
  padding: 8px;
}
.header h4 {
  margin: 5px 0;
}
.header span {
  margin-top: 3px;
}
.list ol {
  border: 1px solid #ccc;
  border-top: none;
  border-radius: 0 0 5px 5px;
  background-color: #fff;            
  list-style-position: inside;
  margin: 0;
  padding: 0;
}
.list ol li {
  border-top: 1px solid #ccc;
  padding: 8px;
  position: relative;
}
.list ol li:first-child {
  border-top: none;
}
.list .btn-group {
  margin-top: 8px;
}
.listname {
  font-weight: bold;  
}
.things li a {
  margin-left:14px;
}
.list a.btn {
  text-align: left;
  border: 1px solid #ccc;
  border-top: none;
  border-radius: 0;
}
.expand, .expandfirst {
  display: block;
  padding: 8px 0;
}
.list a.before-expanded {
  border-bottom-left-radius: 6px;
  border-bottom-right-radius: 6px;
}
.list a.expanded-list, .list a.after-expanded {
  border-top-left-radius: 6px;
  border-top-right-radius: 6px;
  border-top: 1px solid #ccc;
  margin-top: 20px;
}

Jquery

    $(document).ready(function(){
        $(".things").hide();
        $(".listname .btn-group").hide();

        $(".expand, .expandfirst").click(function(){
                $(this).prev(".btn-group").toggle();
                $(this).children("span").toggle();

                $(this).toggleClass('active');
                $(this).closest(".list").find(".things").toggle();
                $(this).parents(".list").next().find(".expand").toggleClass('after-expanded');
                $(this).parents(".list").prev().find(".expand").toggleClass('before-expanded');

                $("#listview, #thingview").removeClass('active');
        });
        $(".expand").click(function(){
                $(this).toggleClass('expanded-list');
        });
        $("#listview").click(function(){
                $(".things").hide();
                $(".listname .btn-group").hide();
                $(".listname a span").show();
                $(".expand").removeClass('active expanded-list after-expanded');                                

        $(".expandfirst").removeClass('active');                                
        });
        $("#thingview").click(function(){
                $(".things").show();
                $(".listname .btn-group").show();
                $(".listname a span").hide();
                $(".expand").addClass('active expanded-list after-expanded');
                $(".expandfirst").addClass('active');
        });
    });
share|improve this question

2 Answers 2

at first glance here's a few things that immediately come to mind (not a criticism, just an observation):

  1. You're right--there's a lot of extra markup to accomplish what you're trying to do, but that's something you can work on easily
  2. Your writing a more jQuery-handlers than you need. You can either wrap this all into a plugin, or use something like jQuery UI or even better, use http://twitter.github.com/bootstrap/ -- which has components that are built to do exactly this.

Now, if you want to roll your own solution (which is awesome too), I suggest you look at using livequery, which gives you the ability to easily bind event handlers to DOM elements. It even handles automatic bindings when new DOM elements are added, so you don't have to repeatedly run initializations for your controls.

An approach I would take for your toggle controls would be this:

<div class="toggle-container state-expanded">
    <div class="toggle-content">
        ... Your content ...
    </div>
    <a href="#" class="toggle-control">Expand/Collapse</a> 
</div>

Your jQuery should look something like this (assuming you go ahead and add livequery):

jQuery(function(){

    $('.toggle-control').livequery( 'click', function(){

        // Store a reference to the element so you don't keep calling $( this ) every time...
        var obj = $( this );
        // References to your elements
        var container = obj.closest('.toggle-container');
        var content = container.find('toggle-content');

        // Expand and collapse the content container
        content.slideToggle(256, function(){
            if(!content.is(':visible') )
            {
                container.addClass('state-collapsed').removeClass('state-expanded');
            }
            else
            {
                container.addClass('state-expanded').removeClass('state-collapsed');
            }
        });

        // Then deal with anything else you want to do with your elements, e.g. show or hide other elements, more initializations etc.
    });



});

I'm probably going to catch some flack for not using toggleClass(), but it's failed me more times that I care to remember, and being slightly more verbose about the conditions provides an opportunity to do more within that condition, e.g. handle the state of other elements.

To address your initial question: the above example can be extended to add/remove classes of all other .toggle-container elements within the slideToggle() callback. This way you don't have to deal with it procedurally.

This approach gives you a chance to reduce the number of CSS rules and pare down your jQuery a little. I would still consider wrapping this all up into a plugin for reusability purposes and to isolate it from your general code.

As an alternative, you can bind the change event for each .toggle-container to respond to changes in the class attribute. Just be mindful of the fact that with the change event, it's possible to have other change events bubble up, which can result in some unwanted behavior. Keep it simple.

share|improve this answer
    
Thanks very much for such a thorough and helpful answer. I did consider using bootstrap 'collapse'. Do you think this is my best option? I suppose I could add to the classes 'collapse' uses to adjust things like margin and border radius when an object is expanded/collapsed? Thanks for your livequery example. I may well go down this road but even if not, I understand a lot more about how to approach this problem so I'm grateful for that. –  user1405195 Oct 14 '12 at 20:58
    
Bootstrap's collapse component is just one option; I suggested it since you're including the CSS for it, so for consistency's sake, it's nice. I tend to roll my own components since most of the plugins out there have more features that I have use-cases for. It's also nice to have a consolidated collection of common controls of your own--if only for the fact that you know how they're built and how to modify them if you need more features (livequery is and awesome tool for these types things.) –  user916011 Oct 14 '12 at 21:11
    
I thought I would have a go at using bootstrap collapse - can you tell me if I'm on the right track here? jsfiddle.net/9SXLJ/4 I've added the line: $this[$(target).hasClass('in') ? 'removeClass' : 'addClass']('active') to the Collapse Data-API and I've used the classes 'collapsed' and 'in' to adjust margin and border-radius. The only part I'm struggling to implement now is adjusting the margin and border-radius for the previous and next objects. –  user1405195 Oct 15 '12 at 1:27
    
I understand your approach, but it's not a good practice to alter a third-party plugin, since you'll lose the benefit of consistency when it's updated, e.g. you'll be forced to re-add your snippet every time there's a new version of the plugin. I learned that the hard way back in the day. The Collapse plugin has a callback parameter specified in, transition: function (method, startEvent, completeEvent){...}, see completeEvent. Hook this event with your own function/handler when you initialize the plugin. Alternately, you can extend/add or even override methods to inject your own logic. –  user916011 Oct 15 '12 at 3:12
    
Oh, and to reset/update the prev/next elements -- I would handle that (assuming you'll hook the completeEvent or startEvent event) by targeting all the .list elements and removing the .active class from the toggle, then applying the .active class to the single list you expanded. –  user916011 Oct 15 '12 at 3:23

This is not really a direct answer, but mostly me trying a slightly different approach. I just wanted to see how far one can get with more CSS and less JS. In fact, I ended up with only 4-5 lines of JS (well, CoffeeScript actually), but this is admittedly for a demo that's heavily simplified compared to the question.

The demo is here

The main point is that only 2 classes are really necessary: expanded and precedes-expanded. The rest can be handled with CSS2's first-child/last-child and + selectors. CSS doesn't provide a way to manipulate preceding elements (i.e. parents or previous siblings), otherwise just one class would be necessary.

Note, though, that this demo probably isn't terribly cross-browser compatible (works great in WebKit, though). Some of it is just a matter of including the vendor-specific syntax for stuff like border-radius, but the selectors mentioned above don't have perfect support everywhere.

share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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