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

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.

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.

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.

Going with my suggestion above, this would all be replaced with:

$(object).toggleClass("collapsed");

Of course, $(object) would need to be the box element.

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.

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>

No spaces within a {{var}}

Here too.

What's the <a> for?

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).

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.

Should be able to use latest() here too, instead of iterating, since that would mean more data going over the wire.

With the cached variable above, this becomes faster. No extra iteration, just comparing against latest_timestamp.

I'd prefer if you used "None" instead of 0. Then your tests can just be:

if latest_timestamp:
    ....

Same here.

And one more test here.

I've gone ahead and made the few tweaks left, and pushed to master as 671c0c7. Thanks!!