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)

enhancement

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)

In the dark theme:
Sidebar background color is var(--grey-90)
Accordion header background color is #141416
Attached patch 1397133.patch [1.0] (obsolete) — Splinter Review
Attachment #8904858 - Flags: review?(pbrosset)
Attached patch 1397133.patch [2.0] (obsolete) — Splinter Review
Attachment #8904858 - Attachment is obsolete: true
Attachment #8904858 - Flags: review?(pbrosset)
Attachment #8904860 - Flags: review?(pbrosset)
Attachment #8904860 - Flags: review?(pbrosset) → review?(bgrinstead)
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)
Attached patch 1397133.patch [3.0] (obsolete) — Splinter Review
Attachment #8904860 - Attachment is obsolete: true
Attachment #8905183 - Flags: review?(bgrinstead)
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-
(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.
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.
Attachment #8905183 - Flags: review- → review?(bgrinstead)
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)
Attachment #8905183 - Attachment is obsolete: true
Attachment #8905246 - Flags: review?(bgrinstead)
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+
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
https://hg.mozilla.org/mozilla-central/rev/83228a8eab33
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.