Closed
Bug 1397133
Opened 7 years ago
Closed 7 years ago
Adjust the sidebar and accordion background colors for the dark theme
Categories
(DevTools :: General, enhancement, P3)
DevTools
General
Tracking
(firefox57 fixed)
RESOLVED
FIXED
Firefox 57
Tracking | Status | |
---|---|---|
firefox57 | --- | fixed |
People
(Reporter: gl, Assigned: gl)
References
(Blocks 1 open bug)
Details
Attachments
(1 file, 3 obsolete files)
6.48 KB,
patch
|
bgrins
:
review+
|
Details | Diff | Splinter Review |
In the dark theme: Sidebar background color is var(--grey-90) Accordion header background color is #141416
Assignee | ||
Comment 1•7 years ago
|
||
Attachment #8904858 -
Flags: review?(pbrosset)
Assignee | ||
Comment 2•7 years ago
|
||
Attachment #8904858 -
Attachment is obsolete: true
Attachment #8904858 -
Flags: review?(pbrosset)
Attachment #8904860 -
Flags: review?(pbrosset)
Assignee | ||
Updated•7 years ago
|
Attachment #8904860 -
Flags: review?(pbrosset) → review?(bgrinstead)
Comment 3•7 years ago
|
||
Comment on attachment 8904860 [details] [diff] [review] 1397133.patch [2.0] Review of attachment 8904860 [details] [diff] [review]: ----------------------------------------------------------------- I haven't reviewed the actual color changes but am assuming that they are matching a spec. This looks good besides the two inline comments ::: devtools/client/inspector/layout/components/Accordion.css @@ +6,5 @@ > * This file should not be modified and is a duplicate from the debugger.html project. > * Any changes to this file should be imported from the upstream debugger.html project. > */ > > +:root.theme-light { Shouldn't this just be `:root` (without the theme-light), to ensure that the variable is never undefined ::: devtools/client/themes/rules.css @@ +18,5 @@ > .theme-firebug { > --rule-highlight-background-color: #ffee99; > --rule-property-name: darkgreen; > --rule-property-value: darkblue; > --rule-overridden-item-border-color: var(--theme-content-color2); I believe you can specify this rule here: --rule-header-background-color: var(--theme-header-background); Then remove the overridden `background` rule in `.theme-firebug .ruleview-header`
Attachment #8904860 -
Flags: review?(bgrinstead)
Assignee | ||
Comment 4•7 years ago
|
||
Attachment #8904860 -
Attachment is obsolete: true
Attachment #8905183 -
Flags: review?(bgrinstead)
Comment 5•7 years ago
|
||
Comment on attachment 8905183 [details] [diff] [review] 1397133.patch [3.0] Review of attachment 8905183 [details] [diff] [review]: ----------------------------------------------------------------- ::: devtools/client/inspector/layout/components/Accordion.css @@ +2,5 @@ > * License, v. 2.0. If a copy of the MPL was not distributed with this > * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ > > /** > * This file should not be modified and is a duplicate from the debugger.html project. I just noticed this comment. Is this still true? If so, this should land in debugger.html first and then be copied into here. If it's not then the comment should be removed.
Attachment #8905183 -
Flags: review?(bgrinstead) → review-
Assignee | ||
Comment 6•7 years ago
|
||
(In reply to Brian Grinstead [:bgrins] from comment #5) > Comment on attachment 8905183 [details] [diff] [review] > 1397133.patch [3.0] > > Review of attachment 8905183 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: devtools/client/inspector/layout/components/Accordion.css > @@ +2,5 @@ > > * License, v. 2.0. If a copy of the MPL was not distributed with this > > * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ > > > > /** > > * This file should not be modified and is a duplicate from the debugger.html project. > > I just noticed this comment. Is this still true? If so, this should land in > debugger.html first and then be copied into here. If it's not then the > comment should be removed. These changes are actually synced the other way around right now. I will be making the same changes in the PR for https://github.com/devtools-html/debugger.html/issues/3909.
Assignee | ||
Comment 7•7 years ago
|
||
It's also not a true sync since the accordion in the debugger.html also has svg and buttons, which we chose to not include and we actually had to implement our own arrow because we couldn't reuse the inline svg.
Assignee | ||
Updated•7 years ago
|
Attachment #8905183 -
Flags: review- → review?(bgrinstead)
Comment 8•7 years ago
|
||
Comment on attachment 8905183 [details] [diff] [review] 1397133.patch [3.0] Review of attachment 8905183 [details] [diff] [review]: ----------------------------------------------------------------- Please update the comment to indicate the reversed direction and include / link to instructions for doing the sync
Attachment #8905183 -
Flags: review?(bgrinstead)
Assignee | ||
Comment 9•7 years ago
|
||
Attachment #8905183 -
Attachment is obsolete: true
Attachment #8905246 -
Flags: review?(bgrinstead)
Comment 10•7 years ago
|
||
Comment on attachment 8905246 [details] [diff] [review] 1397133.patch [4.0] Review of attachment 8905246 [details] [diff] [review]: ----------------------------------------------------------------- ::: devtools/client/inspector/layout/components/Accordion.css @@ +3,5 @@ > * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ > > /** > + * This file is a duplicate from the debugger.html project and has diverged in terms of > + * some of styles that are kept. Any changes to the styles in this file should be sync Nit: 'synced', and trailing whitespace
Attachment #8905246 -
Flags: review?(bgrinstead) → review+
Comment 11•7 years ago
|
||
Pushed by gabriel.luong@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/83228a8eab33 Adjust the sidebar and accordion background colors for the dark theme. r=bgrins
Comment 12•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/83228a8eab33
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox57:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•