Rework much of the Review Board UI, and move to jQuery

Review Request #659 — Created Dec. 3, 2008 and submitted

Information

Review Board SVN (deprecated)
trunk
122, 248, 271, 297, 391, 392, 401, 411, 452, 497, 498, 523, 525, 555, 588, 589, 600, 615, 624, 632, 648, 656, 667, 696, 737, 779

Reviewers

This introduces many significant UI changes to Review Board. The initial
goal was to migrate our UI from YUI/YUI-Ext to jQuery. Along the way, I
took the opportunity to fix a number of usability issues discovered over
the past year or two.

The comment/review dialog has been split up and simplified. The old
dialog was confusing to many users for a number of reasons. They tried
using it to reply to existing comments, they would sometimes lose
comments because they assumed publishing a review would preserve them,
and the dialog was just bulky and would scroll on click if it didn't fit
on the page (due to a YUI-Ext bug).

Now we have a simple comment dialog for both the diff viewer and
screenshot dialogs. The green portion representing the new draft comment
provides a single text area for the comment, and Save and Cancel
buttons. To the left is a list of existing comments for that region (if
any). Each item contains "View" and "Reply" buttons. Clicking these will
jump to the reviews page, and in the case of "Reply," a new comment will
be added below the comment being replied to.

The review portion of the dialog is now its own entity. When clicking on
"Review" on the review request details box, or when clicking "Edit
Review" on the new draft banner (which is docked to the top of a review request
page when there's a pending review), a modal box will pop up containing
the review.

This new box is a vast improvement over the old review dialog. Like
actual reviews, the screenshot and diff fragments are displayed inline,
progressively loaded, and the comments tied to those can be editted
straight from that page. Users can make changes and decide to save them,
publish, discard, or cancel what they've changed.

It's harder to lose comments now. When there's a pending comment on a
page and the user attempts to move away from the page, a confirmation
dialog will appear. This currently happens on the diff viewer and
screenshot dialogs only. The functionality will be added in a later
change to reviews and the review form.

There's more feedback now when making changes. When saving or deleting a
comment, little bubbles briefly float up near the comment flag or
region and let the user know what just happened.

Hovering over a comment flag or a comment region now shows a little
tooltip with the comments. It's brief. No author information is shown,
just the text. Clicking the area provides that additional information.

We provide slightly better error feedback now. The error banners at the
top of the page now contain any debug information, such as a backtrace,
when something goes wrong. In DEBUG=True installs, this information is
very valuable. Certainly makes debugging Review Board itself much
easier.

Review request actions ("Set Submitted", "Add Screenshot", etc.) have
been moved to the top of the review request box where they're more
easily seen. "Set Submitted," "Discard" and "Delete" are no longer
top-level actions. They now exist in a "Close" drop-down menu. This
saves space and groups them together under one concept -- you're in some
way closing a review request out.

On top of all this, 25 bugs have been fixed.
Tested on Firefox 3, and test most of this on IE6/7 and Opera. Some things have changed since my last non-Firefox testing, and I'll be doing more of that before this goes in to make sure it all works. Will be slowly building up some unit tests for this, but it won't be ready right away.
chipx86
CU
  1. I haven't peered through the code yet, but here are some comments just from playing with it.
    
    Overall, I'm very impressed -- this is a nice cleanup that I'm excited to roll out to my users.  It should address most of the usability quirks that people complain to me about.
    
    On the new Close action: clicking it opens a dropdown, so I would expect clicking again should hide the dropdown.  Turning this into a hover instead might also be nice.
    
    There's really no reason to only show Download Patch on the View Diff page.  Can we get it on the View Review page too?
    
    Also, now that they're next to each other, it looks strange to call it a patch in one place and a diff everywhere else.  It should probably be Download Diff for consistency.
    
    The border highlight when browsing the diff is much nicer and more obvious than the old cursor.  It's a little offset when I view it on IE7 though.
    
    Expanding hidden lines isn't working for me.  The error banner says "Error: undefined 200 OK", and when I view details, it looks like the fragment html that I would expect...
    
    The view/reply on existing comments in the diff still feels clunky.  When I click a comment, and see it displayed with a text box next to it, it feels like I should be able to reply right there.  Can you work up a way for people to reply to comments inline, instead of being redirected to the main review page?
    1. Thanks for taking a look and trying it out. All good comments.
      
      The Close menu issue was just an oversight. I tested clicking elsewhere on the page but I didn't test the Close menu. You're right that a hover might just be better.
      
      You're right about the Download Patch being inconsistent. I'll change it to Download Diff. I can also put it on all relevant pages.
      
      There's probably a few browser-specific quirks left to fix. I'm going to go through and test IE6/7 and Opera before this goes in to make sure they work correctly. The issue with IE7 is probably a new regression due to another fix I put in recently.
      
      I don't know why expanding hidden lines would suddenly break but I'll look at it.
      
      As for the replying in the diff viewer, it's definitely not perfect. I talked to David about this earlier in the development and he said the same thing. I have a plan for improving this (and I'd love your opinion on it) but it won't be part of this change. It's already massive as it is. I'm going to work on a better replying mechanism and put that up for review after this one goes in.
      
      So my plan for replying is to have the comment sidebar work sort of like tabs. If there's existing comments, that pane will display and there will be a hard-coded entry at the top saying "New Comment" or something. It will be green, same as the comment area. The existing comments will be clickable and, when clicked, they'll appear as selected and the comment area will turn blue, along with text saying it's a reply. That way, commenting can be done right there, and it's hopefully more clear as to what this comment is: A brand new comment, or a reply to something.
      
      I don't know if that was a good enough description of this... I'll try to reword it or do a mockup if not.
      
      What do you think?
    2. For the expanding lines, FireBug shows an error message in some of your new code, so I imagine it should be easy to track down.  Something about not having a replace method on a retrieved object...
      
      I think I understand your new idea, but it would still be good to see some mockups.  I'm picturing it as the comment dialog box having some pullout tabs on the left, a green one for a new comment, and blue ones for each existing comment with author, date, and a brief part of the actual comment.  When selected, their comment can be shown in full in the main part of the dialog, with the edit box beneath it so it feels like you're replying.
  2. That's all I have for now, but I'll keep playing with it...
david
  1. 
      
  2. Can you add an inline comment explaining what 27 is?
    
    
    Actually, this looks a lot like our old comments code.  What's up?
  3. /trunk/reviewboard/htdocs/media/rb/js/common.js (Diff revision 2)
     
     
     
     
     
     
     
     
     
    This is only really useful if DEBUG = True.  If not, they get a useless error page and the actual error is emailed to the ADMINs
    1. I'll work on something for this.
  4. This doesn't look like it's indented right.
    1. It's all spaces, and it renders fine in Review Board for me. I remember you had this issue once before. Can't remember if we figured it out, though.
  5. There's an extra space here between top: and offset
  6. /trunk/reviewboard/htdocs/media/rb/js/reviews.js (Diff revision 2)
     
     
     
     
     
     
     
     
     
     
    Indentation here is funky.
  7. Can you add this before you submit? It seems pretty important to me.
  8. Shouldn't this be <ul></ul>?
  9. /trunk/reviewboard/templates/diffviewer/view_diff.html (Diff revision 2)
     
     
     
     
     
     
     
    Can we eventually compress these all into one file?
  10. Ship It should be marked as translatable.
    1. Fixed this and the other translatables.
  11. Delete comment should be marked for translation.
  12. This file has a bunch of tabs in it mussing up your indentation.
  13. /trunk/reviewboard/webapi/json.py (Diff revision 2)
     
     
     
    Err.  Shouldn't we maintain backwards-compatibility *after* 1.0 but not before?
    1. It's possible that some people are using ?query in third party code. Unlikely, but possible. At 1.0, I think we should get rid of support for it and require ?q.
  14. /trunk/reviewboard/webapi/json.py (Diff revision 2)
     
     
     
    Same question here.
  15. 
      
chipx86
OR
  1. Good work!
    Here's a few very minor things I saw after a quick look at r3.
  2. Another one.
  3. Getting there.. ;)
    1. Shocked at how badly I screwed some of those up.
  4. /trunk/reviewboard/reviews/views.py (Diff revision 3)
     
     
    Left-over debugging?
  5. /trunk/reviewboard/templates/base.html (Diff revision 3)
     
     
    More left-overs?
    1. I initially was going to do some work for integrating firebug support with error banners, but decided against it. Removing this.
  6. 
      
CU
  1. 
      
  2. /trunk/reviewboard/accounts/backends.py (Diff revision 3)
     
     
    This whole file seems to be reverting r1625 -- probably not meant to be part of this review?
    1. Strange.. Will fix.
  3. I'm getting status = "parsererror" here -- I think it is trying to parse the diff fragment as json.  I added a dataType:"html" to the call, which let me have status = "success", but the fragment still doesn't show up.  Ideas?
    1. Hrm, I haven't seen this. Do you know of a good repro case? Is it all files? Also, what browser?
      
      I'll make the change to specifically set dataType: "html".
    2. False alarm -- turns out I had a caching problem.  After I restarted memcached, all is good.
      
      I guess the cache contents of the diff needed to change, even though the cache lookup key remains the same.  Is there a way that you can put some kind of versioning in the key to prevent this sort of accident?  Or maybe I should just always remember to clear the cache...
    3. We could version it. We need to come up with a good plan for that and we also need to be careful about hitting the max key length.
      
      This change is big enough that I think people will just have to clear the cache anyway, but we should definitely look into key versioning. Could even just have a global value in settings that is prepended to each key.
  4. Your change here is innocent, but code patterns like this could be compressed to "comments.setdefault(key, []).append({...})", or you could make "comments" a "defaultdict(list)" and then append without worry.
    1. Good points. Sadly, I can't use defaultdict because it's not available in Python 2.4, but the first works well enough.
  5. 
      
chipx86
chipx86
chipx86
chipx86
CU
  1. Christopher Orr's earlier comments still need to be addressed, but otherwise it looks good to me!
  2. 
      
chipx86
Review request changed
david
  1. 
      
  2.