Summary

Redesign the look and feel of the review requests page.

Review Request #7240 — Created April 22, 2015 and submitted May 1, 2015, 5:15 p.m.

Information

Owner

chipx86

Repository

Review Board

Branch

release-2.5.x

Bugs

Depends On

Commit

1c8a76e...

Reviewers

Groups

reviewboard

People

Description

The review request page was, in many ways, largely unchanged from the
1.0 days. We've had tweaks here and there, and some layout changes, but
the actual review portion has hardly been touched in years.

This is the first big redesign of this page we've had. It substantially
changes the look and feel, removing a lot of the boxes, toning down
gradients, tightening some spacing and expanding others, and bringing a
more modern look to the page. There are also usability enhancements over
what we had in the past.

In terms of style polish, the big highlights are that there's more
breathing room between reviews, between comments, and between the
content and the browser's boundaries. There are no longer an extra set
of borders within each review for the content, but rather the body of the
review is flat white, providing a cleaner feel. The action bar at the
top no longer has borders separating each action, opting instead to have
appropriate spacing between the items.

There are a few other major changes added on to this as well.

First, the actions on the page no longer include the page views (View
Diff, View Reviews, etc.). Those have been moved just to the right of
the action bar, as a set of tabs. The current page always has an entry
now, making it clear where you are and where you can go.

There are now gravatars shown to the left of the reviews, and to the
left of any comments. The ones on the reviews also contain the
"Ship it!" label, rather than having that appear on reviews. If there
are any issues opened, a "Fix it!" label will show instead. If it's in a
"Fix it, then ship it!" stage, we show a "Fix it!" label sitting on top
of a "Ship it!" label. Any changes to these labels (as the result of
closing/reopening issues) animates the label state, providing a clean,
cute transition.

The border around the gravatars are faintly dotted, with the color
matching the Ship It or Fix It state. This also transitions when the
labels do.

The green reply draft banners now extend up to the top of the review
box. This is quite nice when looking at a collapsed or expanded box.

The timestamps in the page now show only the relative times, as hovering
over them already shows the absolute times. That removes a lot of visual
noise from the page.

The "Add Comment" links on the right have been replaced with "Reply" or
"Comment" buttons on the left. The difference in text depends on whether
there's existing content being replied to.

The "Expand All"/"Collapse All" links (and equivalent links in the diff
viewer) no longer sit in the bottom-right of the review request box, but
rather are prominent btutons separating the review request and reviews.
Likewise, for the diff viewer, they separate the box and the diffs.

Testing Done

Tested all the functionality and browsed through all affected pages in
Chrome and Firefox. My IE VM has been nuked, so I'll be testing that once
I get that going again.

This functionality included:

Expanding/collapsing reviews.
Replying to comments.
Toggling issue states and watching what the labels did (in both the "Fix It" and
the "Fix it, then ship it" states.
Invoking actions from the action bar.
Navigating between tabs for review requests, diffs, and file attachments.
The floating reply draft banners.
The Expand All/Collapse All buttons.
The various view buttons for diffs (Show Whitespace Changes, etc.).
Expanding diffs in the review comments.

(Note that the animation image attached has some color palette goofiness, due
to being a gif, so ignore that aspect of it.)

Files

Issues

Description	From	Last Updated
Can we use the same + and - icons as in the review boxes?	david	April 30, 2015, 11:42 p.m.
The height of the gravatars means that these boxes are pretty tall, which makes it less obvious at a glance …	david	May 1, 2015, 12:14 a.m.
'django_reset' imported but unused	reviewbot	April 22, 2015, 5:08 p.m.
'from settings_local import *' used; unable to detect undefined names	reviewbot	April 22, 2015, 5:08 p.m.
'PIPELINE_CSS' imported but unused	reviewbot	April 22, 2015, 5:08 p.m.
'PIPELINE_JS' imported but unused	reviewbot	April 22, 2015, 5:08 p.m.
Should start with a capital I. This whole comment is a little bit confusing, though. How about: If there are …	david	April 30, 2015, 11:43 p.m.
Mind attaching a screenshot of the file review UI?	david	April 30, 2015, 11:44 p.m.
'django_reset' imported but unused	reviewbot	May 1, 2015, 12:38 a.m.
'from settings_local import *' used; unable to detect undefined names	reviewbot	May 1, 2015, 12:38 a.m.
'PIPELINE_JS' imported but unused	reviewbot	May 1, 2015, 12:38 a.m.
'PIPELINE_CSS' imported but unused	reviewbot	May 1, 2015, 12:38 a.m.

Tool: Pyflakes
Processed Files:
    reviewboard/reviews/views.py
    reviewboard/settings.py
    reviewboard/reviews/templatetags/reviewtags.py

Ignored Files:
    reviewboard/templates/reviews/review_detail.html
    reviewboard/templates/reviews/review_reply_section.html
    reviewboard/templates/reviews/boxes/review.html
    reviewboard/templates/reviews/ui/base.html
    reviewboard/templates/base.html
    reviewboard/static/rb/js/pages/views/reviewRequestPageView.js
    reviewboard/static/rb/css/ui/datagrids.less
    reviewboard/templates/reviews/review_issue_summary_table.html
    reviewboard/templates/reviews/boxes/change.html
    reviewboard/static/rb/js/views/floatingBannerView.js
    reviewboard/templates/reviews/review_reply.html
    reviewboard/templates/diffviewer/view_diff.html
    reviewboard/static/rb/css/pages/base.less
    reviewboard/static/rb/css/ui/boxes.less
    reviewboard/static/rb/css/pages/reviews.less
    reviewboard/static/rb/js/views/reviewBoxView.js
    reviewboard/static/rb/js/views/reviewReplyEditorView.js
    reviewboard/static/rb/css/defs.less
    reviewboard/static/rb/js/models/userSessionModel.js
    reviewboard/static/rb/js/pages/views/diffViewerPageView.js



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/reviews/views.py
    reviewboard/settings.py
    reviewboard/reviews/templatetags/reviewtags.py

Ignored Files:
    reviewboard/templates/reviews/review_detail.html
    reviewboard/templates/reviews/review_reply_section.html
    reviewboard/templates/reviews/boxes/review.html
    reviewboard/templates/reviews/ui/base.html
    reviewboard/templates/base.html
    reviewboard/static/rb/js/pages/views/reviewRequestPageView.js
    reviewboard/static/rb/css/ui/datagrids.less
    reviewboard/templates/reviews/review_issue_summary_table.html
    reviewboard/templates/reviews/boxes/change.html
    reviewboard/static/rb/js/views/floatingBannerView.js
    reviewboard/templates/reviews/review_reply.html
    reviewboard/templates/diffviewer/view_diff.html
    reviewboard/static/rb/css/pages/base.less
    reviewboard/static/rb/css/ui/boxes.less
    reviewboard/static/rb/css/pages/reviews.less
    reviewboard/static/rb/js/views/reviewBoxView.js
    reviewboard/static/rb/js/views/reviewReplyEditorView.js
    reviewboard/static/rb/css/defs.less
    reviewboard/static/rb/js/models/userSessionModel.js
    reviewboard/static/rb/js/pages/views/diffViewerPageView.js

reviewboard/settings.py (Diff revision 1)
The issue has been dropped. Show all issues
```
 'django_reset' imported but unused
```
reviewboard/settings.py (Diff revision 1)
The issue has been dropped. Show all issues
```
 'from settings_local import *' used; unable to detect undefined names
```
reviewboard/settings.py (Diff revision 1)
The issue has been dropped. Show all issues
```
 'PIPELINE_CSS' imported but unused
```
reviewboard/settings.py (Diff revision 1)
The issue has been dropped. Show all issues
```
 'PIPELINE_JS' imported but unused
```

Review Request page (Revision 1)

The issue has been dropped. Show all issues

Can we use the same + and - icons as in the review boxes?

chipx86 April 23, 2015, 7:17 p.m.

I had that originally, and it looked ugly. I tried using these instead for the review boxes, but then they didn't stand out.

I think in this context, it's clear that the whole thing is a button. On the review, it's clear that that's a button. The "+" and "-" are consistent between the two. Putting a button look on another button is probably not going to benefit us in any way, I don't think.

Review Request page (Revision 1)

The issue has been dropped. Show all issues

The height of the gravatars means that these boxes are pretty tall, which makes it less obvious at a glance that there's hidden content. I wonder if we can use that extra vertical space and give the bottom a jagged edge (excuse my crummy ascii art):

----------------------------------
| + Christian Hammond            |
vvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvv

This way it would be more immediately obvious that something was hidden.

chipx86 April 23, 2015, 7:17 p.m.

Hmm, I'm not really sure how we'd do that.

This is why I went with a different color scheme for collapsed boxes (though the green doesn't take advantage of it), to help differentiate from expanded ones.

I'll think about what I can do here.

chipx86 April 30, 2015, 11:44 p.m.

I've tried a handful of things, and I don't like any of them, implementation-wise. CSS doesn't make it easy. People have discovered hacky ways to make the jagged edges work, but you can't give them borders or proper shadows, so they look crappy.

I also tried using dotted and dashed lines. It looks more broken/flawed than helpful.

I'll continue to think about this. For now, can we defer this as future work?

david May 1, 2015, 12:04 a.m.

Sure. Please make sure that's tracked somewhere.

reviewboard/static/rb/js/views/reviewBoxView.js (Diff revision 1)

The issue has been resolved. Show all issues

Should start with a capital I. This whole comment is a little bit confusing, though.

How about:

If there are open issues, there will be a "Fix it!" label.

If there's a Ship It, there will be a "Ship it!" label.

If there's both a Ship It and open issues, the "Fix it!" label will be shown overlaid on top of the "Ship it!" label, and will go away once the issues are resolved.

reviewboard/templates/reviews/ui/base.html (Diff revision 1)

The issue has been resolved. Show all issues

Mind attaching a screenshot of the file review UI?

Christian, a couple of comments from toying with the new UI:

Very cool!
The gravatar's do not seem to respect the "Use Gravatar images" config setting, in that when I disable the setting I still see gravatars. This applies to the datagrid changes in 7216 too.
In Firefox (Windows 37.0.1 or Linux 31.3.0) I do not see the faintly dotted border around the gravatar's, but rather a solid line. In IE 11 I do see faintly dotted. FWIW, I think I prefer the solid line, especially for yellow (ship it) where my eyes have a hard time distinguishing yellow from the gray background.
With the significant increase in amount of gravatar's displayed, I'd like to suggest an admin config setting to select the default gravatar type. It looks like you have set the default to mystery-man, but in an environment where many users have not set up their gravatar I'd prefer to set the default to wavatar or one of the other "dynamic" default types to create a visual distinction between users.

chipx86 April 23, 2015, 8:20 p.m.

Thanks for the feedback!

I'll makee sure to address #2 in an update.

For #3, I'll play with it more. I was intentionally going for a very subtle approach of the dots (given that the labels are the more visually-noticeable elements for the status, I wasn't too concerned about the dots standing out), but I can toy with it. None of this is set in stone, and I expect some of it will change after beta 2 anyway.

For #4, the default can be set in settings_local.py. I think it makes sense to have some UI around this, but I'd want to do that as part of a larger change that reduces the reliability on gravatars, and instead allows companies to link up with whatever internal service they're already using for avatars/employee pictures.

Change Summary:


Factored in some of the new margins when computing the Testing Done field height.
Gravatars are no longer shown if turned off in the admin UI.
Improved the documentation of a function.

Commit:

b3bd26ba2f2fcc47d09bc17cc07147108f97aade

1c8a76eb36a7248fb5bd25db120eec47bcf02e46

Diff:

Revision 2 (+672 -226)

Show changes

	reviewboard/settings.py
	reviewboard/reviews/views.py
	reviewboard/reviews/templatetags/reviewtags.py
	reviewboard/static/rb/css/defs.less
	reviewboard/static/rb/css/pages/base.less
	reviewboard/static/rb/css/pages/reviews.less
	reviewboard/static/rb/css/ui/boxes.less
	reviewboard/static/rb/js/models/userSessionModel.js
	15 more

Added Files:

Tool: Pyflakes
Processed Files:
    reviewboard/reviews/views.py
    reviewboard/settings.py
    reviewboard/reviews/templatetags/reviewtags.py

Ignored Files:
    reviewboard/templates/reviews/review_detail.html
    reviewboard/templates/reviews/review_reply_section.html
    reviewboard/templates/reviews/ui/base.html
    reviewboard/templates/base.html
    reviewboard/static/rb/js/pages/views/reviewRequestPageView.js
    reviewboard/templates/reviews/review_reply.html
    reviewboard/static/rb/js/views/reviewBoxView.js
    reviewboard/templates/reviews/review_issue_summary_table.html
    reviewboard/templates/reviews/boxes/review.html
    reviewboard/static/rb/js/views/floatingBannerView.js
    reviewboard/static/rb/js/views/reviewRequestEditorView.js
    reviewboard/templates/diffviewer/view_diff.html
    reviewboard/static/rb/css/pages/base.less
    reviewboard/static/rb/css/ui/boxes.less
    reviewboard/static/rb/css/pages/reviews.less
    reviewboard/templates/reviews/boxes/change.html
    reviewboard/static/rb/js/views/reviewReplyEditorView.js
    reviewboard/static/rb/css/defs.less
    reviewboard/static/rb/js/models/userSessionModel.js
    reviewboard/static/rb/js/pages/views/diffViewerPageView.js



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/reviews/views.py
    reviewboard/settings.py
    reviewboard/reviews/templatetags/reviewtags.py

Ignored Files:
    reviewboard/templates/reviews/review_detail.html
    reviewboard/templates/reviews/review_reply_section.html
    reviewboard/templates/reviews/ui/base.html
    reviewboard/templates/base.html
    reviewboard/static/rb/js/pages/views/reviewRequestPageView.js
    reviewboard/templates/reviews/review_reply.html
    reviewboard/static/rb/js/views/reviewBoxView.js
    reviewboard/templates/reviews/review_issue_summary_table.html
    reviewboard/templates/reviews/boxes/review.html
    reviewboard/static/rb/js/views/floatingBannerView.js
    reviewboard/static/rb/js/views/reviewRequestEditorView.js
    reviewboard/templates/diffviewer/view_diff.html
    reviewboard/static/rb/css/pages/base.less
    reviewboard/static/rb/css/ui/boxes.less
    reviewboard/static/rb/css/pages/reviews.less
    reviewboard/templates/reviews/boxes/change.html
    reviewboard/static/rb/js/views/reviewReplyEditorView.js
    reviewboard/static/rb/css/defs.less
    reviewboard/static/rb/js/models/userSessionModel.js
    reviewboard/static/rb/js/pages/views/diffViewerPageView.js

reviewboard/settings.py (Diff revision 2)
The issue has been dropped. Show all issues
```
 'django_reset' imported but unused
```
reviewboard/settings.py (Diff revision 2)
The issue has been dropped. Show all issues
```
 'from settings_local import *' used; unable to detect undefined names
```
reviewboard/settings.py (Diff revision 2)
The issue has been dropped. Show all issues
```
 'PIPELINE_JS' imported but unused
```
reviewboard/settings.py (Diff revision 2)
The issue has been dropped. Show all issues
```
 'PIPELINE_CSS' imported but unused
```

Ship it!

```
Ship It!
```

Status:: Completed
Change Summary:: Pushed to release-2.5.x (a8168fd)