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
|
- Commit:
-
8141789b1d78b577bdac0ef79ce01e8967332ebe33ac7a7c4c6c5b945f438be8d5cbdc62d16fcd79
- 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:
-
~ This change makes some progress on cleaning up and refactoring the HTML and CSS
~ related to the review request box. It's also making small progress on shifting ~ the styles responsible for the review request box into review-request.less. The ~ functional changes relate mostly to ensuring consistency throughout with ~ regards to spacing of elements. For example, we had a 1px misalignment between ~ the items in the summary and the rest of the main fieldsets. There was also a ~ kind of ugly misalignment in the "Information" and "Reviewers" panels whereby ~ the pencil next to empty fields was shifted over 4px. ~ 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.
+ + Testing done:
+ 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. - Commit:
-
33ac7a7c4c6c5b945f438be8d5cbdc62d16fcd79c2f27c2d1d74bf93f3cf76f10da4365c08fef091
- 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:
-
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.
- - Testing done:
- 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. - Testing Done:
-
~ Verified the appearance and functionality of the review request box with all
~ sections on both desktop and mobile. ~ 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. - Commit:
-
c2f27c2d1d74bf93f3cf76f10da4365c08fef091d722e11898bc676195d4cec6ef88ac30c8c35871
- 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:
-
d722e11898bc676195d4cec6ef88ac30c8c358717fbd3a68119d1cfa3dbda8c37c86a604582fffc8
- 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
- Commit:
-
7fbd3a68119d1cfa3dbda8c37c86a604582fffc8c906ad145b514a689c09c373c3a910340449df5e
- 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?
-
-
-
-
-
-
-
-
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:
-
c906ad145b514a689c09c373c3a910340449df5e7ea1e3179d4a8b5b08e474648c8e3eb4247fcd24
- 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