Summary

Simplify review thread markup and CSS.

Review Request #8391 — Created Sept. 8, 2016 and submitted Sept. 13, 2016, 10:17 a.m.

Information

Owner

david

Repository

Review Board

Branch

release-3.0.x

Bugs

Depends On

Commit

7108bc7...

Reviewers

Groups

reviewboard

People

Description

This change dramatically improves the markup and CSS for review threads.
Specifically, instead of using <dl> for everything (which isn't semantically
correct), we wrap things in <ol> elements, and then individual comments are
broken up into <div>s with sensible classes. These are then styled with
consistent and simple spacing so that everything looks nice while also being
aligned. Replies to a comment are all indented a bit from that comment,
emphasizing the relationship in the way that most threaded displays work. One
particularly nice part of this change is that the text for comments is now
horizontally aligned with the avatar (the borders of the text area are
accounted for using negative margins).

Testing Done

Tested reviews with all sorts of different comments, and replies to those
reviews. Checked to make sure everything was aligned and spaced correctly.
Opened the editor for draft replies to verify that there were no jumps in the
position of the text.

Files

Issues

Description	From	Last Updated
&.draft should go before any child selectors.	chipx86	Sept. 12, 2016, 10:56 p.m.
Can we add a constant for this color?	chipx86	Sept. 12, 2016, 10:58 p.m.
"could be simpler"	chipx86	Sept. 12, 2016, 10:58 p.m.
No need for the space before data-context-id.	chipx86	Sept. 12, 2016, 10:58 p.m.

Tool: Pyflakes
Processed Files:
    reviewboard/reviews/models/review.py
    reviewboard/reviews/templatetags/reviewtags.py

Ignored Files:
    reviewboard/templates/reviews/review_reply_section.html
    reviewboard/templates/reviews/review_reply.html
    reviewboard/templates/reviews/boxes/review_body.html
    reviewboard/templates/reviews/boxes/review.html
    reviewboard/static/rb/css/pages/reviews.less
    reviewboard/static/rb/js/views/reviewReplyEditorView.js



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/reviews/models/review.py
    reviewboard/reviews/templatetags/reviewtags.py

Ignored Files:
    reviewboard/templates/reviews/review_reply_section.html
    reviewboard/templates/reviews/review_reply.html
    reviewboard/templates/reviews/boxes/review_body.html
    reviewboard/templates/reviews/boxes/review.html
    reviewboard/static/rb/css/pages/reviews.less
    reviewboard/static/rb/js/views/reviewReplyEditorView.js

Good cleanup. Just a few small comments.

reviewboard/static/rb/css/pages/reviews.less (Diff revision 1)

The issue has been resolved. Show all issues

&.draft should go before any child selectors.

david Sept. 12, 2016, 10:59 p.m.

In this case I think I disagree. Ordinarily sure, but this &.draft doesn't have any rules that directly apply to the selected element. It only contains more child selectors. The result is that the first rules define the normal behavior, and these modify those for when there's a draft. Switching the order makes it less comprehensible.

I'll add a comment.

chipx86 Sept. 12, 2016, 11:01 p.m.

It doesn't right now, but if it did later we'd end up creating an inconsistency. We do this exact thing in other places already.

david Sept. 12, 2016, 11:04 p.m.

Even in that case I would still argue for putting the general rules first and the ones that specialize individual aspects of those rules for specific states second.

reviewboard/static/rb/css/pages/reviews.less (Diff revision 1)
The issue has been resolved. Show all issues
```
Can we add a constant for this color?
```
reviewboard/static/rb/css/pages/reviews.less (Diff revision 1)
The issue has been resolved. Show all issues
```
"could be simpler"
```
reviewboard/templates/reviews/review_reply_section.html (Diff revision 1)
The issue has been resolved. Show all issues
```
No need for the space before data-context-id.
```

Change Summary:

Make requested changes.

Commit:

35dde7abf614e37b51fd473f08fb61853c7ea28b

7108bc7bc44b17c23945ccf83b534f042182bf71

Diff:

Revision 2 (+308 -329)

Show changes

	reviewboard/reviews/models/review.py
	reviewboard/reviews/templatetags/reviewtags.py
	reviewboard/static/rb/css/defs.less
	reviewboard/static/rb/css/pages/reviews.less
	reviewboard/static/rb/js/views/reviewReplyEditorView.js
	reviewboard/templates/reviews/review_reply.html
	reviewboard/templates/reviews/review_reply_section.html
	reviewboard/templates/reviews/boxes/review.html
	1 more

Tool: Pyflakes
Processed Files:
    reviewboard/reviews/models/review.py
    reviewboard/reviews/templatetags/reviewtags.py

Ignored Files:
    reviewboard/templates/reviews/review_reply_section.html
    reviewboard/templates/reviews/review_reply.html
    reviewboard/templates/reviews/boxes/review_body.html
    reviewboard/templates/reviews/boxes/review.html
    reviewboard/static/rb/css/pages/reviews.less
    reviewboard/static/rb/js/views/reviewReplyEditorView.js
    reviewboard/static/rb/css/defs.less



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/reviews/models/review.py
    reviewboard/reviews/templatetags/reviewtags.py

Ignored Files:
    reviewboard/templates/reviews/review_reply_section.html
    reviewboard/templates/reviews/review_reply.html
    reviewboard/templates/reviews/boxes/review_body.html
    reviewboard/templates/reviews/boxes/review.html
    reviewboard/static/rb/css/pages/reviews.less
    reviewboard/static/rb/js/views/reviewReplyEditorView.js
    reviewboard/static/rb/css/defs.less

Ship it!

```
Ship It!
```

Status:: Completed
Change Summary:: Pushed to release-3.0.x (c0be5e0)