Clean up markup and CSS for the review request box.
Review Request #8312 — Created July 27, 2016 and submitted
This change does a major cleanup for the HTML and CSS related to the review
request box. There are no real functional changes, but lots of small refactors
and cleanups:
- All styles relating to the review request box now live in review-request.less
- Spacing and alignment has been fixed throughout. Examples of this include a
1px misalignment that we had between the "Summary:" box and the rest of the
boxes, a 4px misalignment on the pencil icon for fields that didn't have any
content, and a big handful of places that just grabbed spacing values out of
a hat. Things now rely on a common "review-request-section" class which sets
the spacing around the box. - IDs in this box now consistently use hyphens, where before we had a mix.
- Use of
remrather thanem(since it's supported on all the browsers we
now care about). - Menus now actually use our common
.has-menuand.menurules, rather than
completely reimplementing them. - Better namespacing for all the action classes, swapping out the very general
.action*for.review-request-action* - Useless elements were removed, including the use of
{% box %}around the
review request, since we were overriding all the box styles anyway. - A bunch of totally obsolete style rules (relevant classes/IDs didn't exist
anymore) were removed.
Tested a variety of browsers on both desktop and mobile for the appearance and
functionality of the review request detail, diff viewer, and review UI pages.
| Description | From | Last Updated |
|---|---|---|
|
Doesn't this generate .field:not(:empty):empty, which can't match anything? |
 brennie
|
|
|
We prefer hex over color names, don't we? |
brennie
|
|
|
Uppercase B. |
brennie
|
|
|
Might be a good candidate for a constant. |
 chipx86
|
|
|
This must use @{STATIC_ROOT}djblets/css/mixins/markdown.less, or packaging will break. |
chipx86
|
|
|
Two blank lines. |
chipx86
|
|
|
Order should be tags, IDs, classes. |
chipx86
|
|
|
Let's define a constant for this. |
chipx86
|
|
|
You can use .desktop-only(); instead. |
chipx86
|
|
|
Tags and then classes. |
chipx86
|
-
-
reviewboard/static/rb/css/pages/review-request.less (Diff revision 1) Doesn't this generate
.field:not(:empty):empty, which can't match anything?
Commit: |
|
||||
|---|---|---|---|---|---|
Diff: |
Revision 2 (+207 -220) |
-
Tool: Pyflakes Processed Files: reviewboard/staticbundles.py Ignored Files: reviewboard/templates/reviews/review_detail.html reviewboard/templates/reviews/review_request_header.html reviewboard/templates/reviews/review_request_box.html reviewboard/templates/diffviewer/view_diff.html reviewboard/templates/reviews/review_issue_summary_table.html reviewboard/static/rb/css/pages/reviews.less reviewboard/templates/reviews/action.html reviewboard/static/rb/css/pages/review-request.less reviewboard/static/rb/css/defs.less reviewboard/templates/reviews/menu_action.html Tool: PEP8 Style Checker Processed Files: reviewboard/staticbundles.py Ignored Files: reviewboard/templates/reviews/review_detail.html reviewboard/templates/reviews/review_request_header.html reviewboard/templates/reviews/review_request_box.html reviewboard/templates/diffviewer/view_diff.html reviewboard/templates/reviews/review_issue_summary_table.html reviewboard/static/rb/css/pages/reviews.less reviewboard/templates/reviews/action.html reviewboard/static/rb/css/pages/review-request.less reviewboard/static/rb/css/defs.less reviewboard/templates/reviews/menu_action.html
Description: |
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Commit: |
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Diff: |
Revision 3 (+500 -623) |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Removed Files: |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Added Files: |
-
Tool: Pyflakes Processed Files: reviewboard/staticbundles.py Ignored Files: reviewboard/templates/reviews/review_detail.html reviewboard/templates/reviews/ui/base.html reviewboard/static/rb/js/views/tests/reviewRequestEditorViewTests.js reviewboard/static/rb/css/pages/diffviewer.less reviewboard/static/rb/css/ui/menus.less reviewboard/templates/reviews/review_request_header.html reviewboard/static/rb/js/views/reviewRequestEditorView.js reviewboard/templates/diffviewer/view_diff.html reviewboard/templates/reviews/review_issue_summary_table.html reviewboard/static/rb/css/pages/reviews.less reviewboard/templates/reviews/action.html reviewboard/templates/reviews/review_request_box.html reviewboard/static/rb/css/pages/review-request.less reviewboard/static/rb/css/defs.less reviewboard/templates/reviews/menu_action.html reviewboard/static/rb/js/pages/views/reviewablePageView.js reviewboard/static/rb/css/mixins/style.less Tool: PEP8 Style Checker Processed Files: reviewboard/staticbundles.py Ignored Files: reviewboard/templates/reviews/review_detail.html reviewboard/templates/reviews/ui/base.html reviewboard/static/rb/js/views/tests/reviewRequestEditorViewTests.js reviewboard/static/rb/css/pages/diffviewer.less reviewboard/static/rb/css/ui/menus.less reviewboard/templates/reviews/review_request_header.html reviewboard/static/rb/js/views/reviewRequestEditorView.js reviewboard/templates/diffviewer/view_diff.html reviewboard/templates/reviews/review_issue_summary_table.html reviewboard/static/rb/css/pages/reviews.less reviewboard/templates/reviews/action.html reviewboard/templates/reviews/review_request_box.html reviewboard/static/rb/css/pages/review-request.less reviewboard/static/rb/css/defs.less reviewboard/templates/reviews/menu_action.html reviewboard/static/rb/js/pages/views/reviewablePageView.js reviewboard/static/rb/css/mixins/style.less
Change Summary:
Move banners and updates bubble rules over, fix description.
Description: |
|
||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Testing Done: |
|
||||||||||||||||||||||||||||||||||||||||||||||||
Commit: |
|
||||||||||||||||||||||||||||||||||||||||||||||||
Diff: |
Revision 4 (+578 -669) |
-
Tool: PEP8 Style Checker Processed Files: reviewboard/staticbundles.py Ignored Files: reviewboard/templates/reviews/review_detail.html reviewboard/templates/reviews/ui/base.html reviewboard/static/rb/js/views/tests/reviewRequestEditorViewTests.js reviewboard/static/rb/css/pages/diffviewer.less reviewboard/static/rb/css/ui/menus.less reviewboard/templates/reviews/review_request_header.html reviewboard/static/rb/js/views/reviewRequestEditorView.js reviewboard/templates/diffviewer/view_diff.html reviewboard/templates/reviews/review_issue_summary_table.html reviewboard/static/rb/css/pages/reviews.less reviewboard/templates/reviews/action.html reviewboard/templates/reviews/review_request_box.html reviewboard/static/rb/css/pages/review-request.less reviewboard/static/rb/css/defs.less reviewboard/templates/reviews/menu_action.html reviewboard/static/rb/js/pages/views/reviewablePageView.js reviewboard/static/rb/css/mixins/style.less Tool: Pyflakes Processed Files: reviewboard/staticbundles.py Ignored Files: reviewboard/templates/reviews/review_detail.html reviewboard/templates/reviews/ui/base.html reviewboard/static/rb/js/views/tests/reviewRequestEditorViewTests.js reviewboard/static/rb/css/pages/diffviewer.less reviewboard/static/rb/css/ui/menus.less reviewboard/templates/reviews/review_request_header.html reviewboard/static/rb/js/views/reviewRequestEditorView.js reviewboard/templates/diffviewer/view_diff.html reviewboard/templates/reviews/review_issue_summary_table.html reviewboard/static/rb/css/pages/reviews.less reviewboard/templates/reviews/action.html reviewboard/templates/reviews/review_request_box.html reviewboard/static/rb/css/pages/review-request.less reviewboard/static/rb/css/defs.less reviewboard/templates/reviews/menu_action.html reviewboard/static/rb/js/pages/views/reviewablePageView.js reviewboard/static/rb/css/mixins/style.less
Change Summary:
Fix js-tests.
Commit: |
|
||||
|---|---|---|---|---|---|
Diff: |
Revision 5 (+581 -672) |
-
Tool: Pyflakes Processed Files: reviewboard/staticbundles.py Ignored Files: reviewboard/templates/reviews/review_detail.html reviewboard/templates/reviews/ui/base.html reviewboard/static/rb/js/views/tests/reviewRequestEditorViewTests.js reviewboard/static/rb/css/pages/diffviewer.less reviewboard/static/rb/css/ui/menus.less reviewboard/templates/reviews/review_request_header.html reviewboard/static/rb/js/views/reviewRequestEditorView.js reviewboard/templates/diffviewer/view_diff.html reviewboard/templates/reviews/review_issue_summary_table.html reviewboard/static/rb/css/pages/reviews.less reviewboard/templates/reviews/action.html reviewboard/templates/reviews/review_request_box.html reviewboard/static/rb/css/pages/review-request.less reviewboard/static/rb/css/defs.less reviewboard/templates/reviews/menu_action.html reviewboard/static/rb/js/pages/views/reviewablePageView.js reviewboard/static/rb/css/mixins/style.less Tool: PEP8 Style Checker Processed Files: reviewboard/staticbundles.py Ignored Files: reviewboard/templates/reviews/review_detail.html reviewboard/templates/reviews/ui/base.html reviewboard/static/rb/js/views/tests/reviewRequestEditorViewTests.js reviewboard/static/rb/css/pages/diffviewer.less reviewboard/static/rb/css/ui/menus.less reviewboard/templates/reviews/review_request_header.html reviewboard/static/rb/js/views/reviewRequestEditorView.js reviewboard/templates/diffviewer/view_diff.html reviewboard/templates/reviews/review_issue_summary_table.html reviewboard/static/rb/css/pages/reviews.less reviewboard/templates/reviews/action.html reviewboard/templates/reviews/review_request_box.html reviewboard/static/rb/css/pages/review-request.less reviewboard/static/rb/css/defs.less reviewboard/templates/reviews/menu_action.html reviewboard/static/rb/js/pages/views/reviewablePageView.js reviewboard/static/rb/css/mixins/style.less
-
-
reviewboard/static/rb/css/pages/review-request.less (Diff revision 5) We prefer hex over color names, don't we?
-
Commit: |
|
||||
|---|---|---|---|---|---|
Diff: |
Revision 6 (+581 -672) |
-
Tool: Pyflakes Processed Files: reviewboard/staticbundles.py Ignored Files: reviewboard/templates/reviews/review_detail.html reviewboard/templates/reviews/ui/base.html reviewboard/static/rb/js/views/tests/reviewRequestEditorViewTests.js reviewboard/static/rb/css/pages/diffviewer.less reviewboard/static/rb/css/ui/menus.less reviewboard/templates/reviews/review_request_header.html reviewboard/static/rb/js/views/reviewRequestEditorView.js reviewboard/templates/diffviewer/view_diff.html reviewboard/templates/reviews/review_issue_summary_table.html reviewboard/static/rb/css/pages/reviews.less reviewboard/templates/reviews/action.html reviewboard/templates/reviews/review_request_box.html reviewboard/static/rb/css/pages/review-request.less reviewboard/static/rb/css/defs.less reviewboard/templates/reviews/menu_action.html reviewboard/static/rb/js/pages/views/reviewablePageView.js reviewboard/static/rb/css/mixins/style.less Tool: PEP8 Style Checker Processed Files: reviewboard/staticbundles.py Ignored Files: reviewboard/templates/reviews/review_detail.html reviewboard/templates/reviews/ui/base.html reviewboard/static/rb/js/views/tests/reviewRequestEditorViewTests.js reviewboard/static/rb/css/pages/diffviewer.less reviewboard/static/rb/css/ui/menus.less reviewboard/templates/reviews/review_request_header.html reviewboard/static/rb/js/views/reviewRequestEditorView.js reviewboard/templates/diffviewer/view_diff.html reviewboard/templates/reviews/review_issue_summary_table.html reviewboard/static/rb/css/pages/reviews.less reviewboard/templates/reviews/action.html reviewboard/templates/reviews/review_request_box.html reviewboard/static/rb/css/pages/review-request.less reviewboard/static/rb/css/defs.less reviewboard/templates/reviews/menu_action.html reviewboard/static/rb/js/pages/views/reviewablePageView.js reviewboard/static/rb/css/mixins/style.less
-
This looks like a nice cleanup change. We will absolutely be breaking users with some of the ID/class renames. In another review request, I mentioned putting together a page on Notion with things that we're changing, so that authors can read through them. Can you document the major ID/class name changes as well (review request and action ones) so we can include them in the release notes, so Mozilla (and other companies I can't mention here) aren't completely in shock and have a decent guide on transitioning these?
-
reviewboard/static/rb/css/pages/diffviewer.less (Diff revision 6) Might be a good candidate for a constant.
-
reviewboard/static/rb/css/pages/review-request.less (Diff revision 6) This must use
@{STATIC_ROOT}djblets/css/mixins/markdown.less, or packaging will break. -
-
reviewboard/static/rb/css/pages/review-request.less (Diff revision 6) Order should be tags, IDs, classes.
-
reviewboard/static/rb/css/pages/review-request.less (Diff revision 6) Let's define a constant for this.
-
reviewboard/static/rb/css/pages/review-request.less (Diff revision 6) You can use
.desktop-only();instead. -
-
reviewboard/templates/reviews/menu_action.html (Diff revision 6) Once upon a time, we had issues with these appearing unless
display: nonewas set. This was probably due to the dynamic nature of how less stylesheets were loaded before, but I just want to verify that the menus never appear unexpectedly on load.
Commit: |
|
||||
|---|---|---|---|---|---|
Diff: |
Revision 7 (+579 -672) |
-
Tool: Pyflakes Processed Files: reviewboard/staticbundles.py Ignored Files: reviewboard/templates/reviews/review_detail.html reviewboard/templates/reviews/ui/base.html reviewboard/static/rb/js/views/tests/reviewRequestEditorViewTests.js reviewboard/static/rb/css/pages/diffviewer.less reviewboard/static/rb/css/ui/menus.less reviewboard/templates/reviews/review_request_header.html reviewboard/static/rb/js/views/reviewRequestEditorView.js reviewboard/templates/diffviewer/view_diff.html reviewboard/templates/reviews/review_issue_summary_table.html reviewboard/static/rb/css/pages/reviews.less reviewboard/templates/reviews/action.html reviewboard/templates/reviews/review_request_box.html reviewboard/static/rb/css/pages/review-request.less reviewboard/static/rb/css/defs.less reviewboard/templates/reviews/menu_action.html reviewboard/static/rb/js/pages/views/reviewablePageView.js reviewboard/static/rb/css/mixins/style.less Tool: PEP8 Style Checker Processed Files: reviewboard/staticbundles.py Ignored Files: reviewboard/templates/reviews/review_detail.html reviewboard/templates/reviews/ui/base.html reviewboard/static/rb/js/views/tests/reviewRequestEditorViewTests.js reviewboard/static/rb/css/pages/diffviewer.less reviewboard/static/rb/css/ui/menus.less reviewboard/templates/reviews/review_request_header.html reviewboard/static/rb/js/views/reviewRequestEditorView.js reviewboard/templates/diffviewer/view_diff.html reviewboard/templates/reviews/review_issue_summary_table.html reviewboard/static/rb/css/pages/reviews.less reviewboard/templates/reviews/action.html reviewboard/templates/reviews/review_request_box.html reviewboard/static/rb/css/pages/review-request.less reviewboard/static/rb/css/defs.less reviewboard/templates/reviews/menu_action.html reviewboard/static/rb/js/pages/views/reviewablePageView.js reviewboard/static/rb/css/mixins/style.less
Tool: Pyflakes Processed Files: reviewboard/staticbundles.py Ignored Files: reviewboard/templates/reviews/review_detail.html reviewboard/templates/reviews/review_request_header.html reviewboard/templates/reviews/review_request_box.html reviewboard/templates/diffviewer/view_diff.html reviewboard/templates/reviews/review_issue_summary_table.html reviewboard/static/rb/css/pages/reviews.less reviewboard/templates/reviews/action.html reviewboard/static/rb/css/pages/review-request.less reviewboard/static/rb/css/defs.less reviewboard/templates/reviews/menu_action.html Tool: PEP8 Style Checker Processed Files: reviewboard/staticbundles.py Ignored Files: reviewboard/templates/reviews/review_detail.html reviewboard/templates/reviews/review_request_header.html reviewboard/templates/reviews/review_request_box.html reviewboard/templates/diffviewer/view_diff.html reviewboard/templates/reviews/review_issue_summary_table.html reviewboard/static/rb/css/pages/reviews.less reviewboard/templates/reviews/action.html reviewboard/static/rb/css/pages/review-request.less reviewboard/static/rb/css/defs.less reviewboard/templates/reviews/menu_action.html