Collapsible reviews.

Review Request #1832 — Created Oct. 16, 2010 and submitted

Information

Review Board

Reviewers

In order to tidy existing and future review requests I have implemented collapsible reviews.  There are two criteria for the starting state of a review.

a) The review is collapsed if it is older than the last change to the review request.

b) The review is collapsed if all of its replies are older than the last time the user visited the page.

I wanted to use cookies to allow the site to remember which reviews had been collapsed or expanded, but couldn't figure out a good way of implementing it.  Hopefully I can make this available later.
Manual testing.

Screenshots

KF
mike_conley
  1. Kevin:
    
    Solid work here - and that screenshot looks awesome.  I'm looking forward to seeing this merged!  :D
    
    Just a few notes - see below.  Nothing major.
    
    Thanks,
    
    -Mike
  2. reviewboard/reviews/views.py (Diff revision 1)
     
     
    changedescs.latest('timestamp') throws a DoesNotExist exception if there are no change descriptions for a review request - so you'll need to do a check for that.
  3. reviewboard/reviews/views.py (Diff revision 1)
     
     
    I've noticed that there's a RB code tendency to have a trailing comma on the last element of a list (see line 185 on left of side by side).  Might as well keep that up - maybe put a comma after state.
  4. 
      
chipx86
  1. 
      
  2. reviewboard/htdocs/media/rb/css/reviews.css (Diff revision 1)
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
    Given that most of this is identical, it'd be nice to be able to combine the common parts into one rule. Either putting the expand/collapse element into some known parent element and apply the rule to that, or attach an additional class to the element.
    
    I think I'd actually like to see this changed a bit. Rather than name the button expand or collapse, I'd like to see the box's state represented. So, an expanded box wouldn't really have a class, but if it's collapsed, it'd have a "collapsed" class on it.
    
    Then this area would have a "collapser-button" class, which would contain all the stuff currently in ".expand". Then, you'd end up with the following rules:
    
    .collapser-button {
        /* Stuff currently in .expand */
    }
    
    .collapsed .collapser-button {
        background: url("../images/expand.png") no-repeat;
    }
    
    Now instead of two classes representing two possible views for the button, you have one, and you're just changing the image when the state of the box changes.
    
    This will, of course, require that you change which element you're applying the class on.
  3. reviewboard/htdocs/media/rb/js/reviews.js (Diff revision 1)
     
     
     
     
     
     
    Going with my suggestion above, this would all be replaced with:
    
    $(object).toggleClass("collapsed");
    
    Of course, $(object) would need to be the box element.
  4. reviewboard/htdocs/media/rb/js/reviews.js (Diff revision 1)
     
     
     
     
    So, given this, I assume that the reviews all appear expanded for a brief moment as the page loads?
    
    Using the "collapsed" class I suggested above, could this change so that the elements within are hidden through a CSS rule? Something like:
    
    .collapsed .main { display: none; }
    
    That would save on these.
  5. IDs should use - instead of _
    
    Also, for this line, please do what we do for the diff viewer for the expand/collapse buttons, in that you should have a <ul class="controls"> with the controls inside.
    
    You also shouldn't put the JavaScript in the href="", and this should be localized. So:
    
    <ul class="controls">
     <li><a href="#" id="expand-all">{% trans "Expand All" %}</a></li>
    </ul>
  6. No spaces within a {{var}}
  7. Here too.
  8. What's the <a> for?
  9. 
      
KF
chipx86
  1. This is really looking awesome. I can't wait to get this in. I have a couple suggestions for reducing database lookups, though (something I'm setting as a strong goal for this release).
  2. reviewboard/reviews/views.py (Diff revision 2)
     
     
     
     
    You should grab this only once. Otherwise, it may end up doing more queries than we want.
    
    You can do:
    
      try:
          latest_changedesc = changedescs.latest('timestamp').only('timestamp')
          latest_timestamp = latest_changedesc.timestamp
      except ChangeDescription.DoesNotExist:
          latest_timestamp = 0
    
    And then use the latest_timestamp variable for comparisons.
  3. reviewboard/reviews/views.py (Diff revision 2)
     
     
     
    Should be able to use latest() here too, instead of iterating, since that would mean more data going over the wire.
  4. reviewboard/reviews/views.py (Diff revision 2)
     
     
     
    With the cached variable above, this becomes faster. No extra iteration, just comparing against latest_timestamp.
  5. 
      
KF
Review request changed
Change Summary:
Changed style. (access database less)
Slightly modified js. (nothing important)
Fixed a bug with 'expand all' (bug with expanding a review change).
david
  1. 
      
  2. reviewboard/reviews/views.py (Diff revisions 2 - 3)
     
     
     
     
     
    I'd prefer if you used "None" instead of 0. Then your tests can just be:
    
    if latest_timestamp:
        ....
  3. reviewboard/reviews/views.py (Diff revisions 2 - 3)
     
     
     
     
     
    Same here.
  4. reviewboard/reviews/views.py (Diff revisions 2 - 3)
     
     
    And one more test here.
  5. 
      
SA
  1. 
      
  2. 
      
david
  1. I've gone ahead and made the few tweaks left, and pushed to master as 671c0c7. Thanks!!
  2.