Polish dark theme for the application panel

RESOLVED FIXED in Firefox 62

Status

enhancement
RESOLVED FIXED
a year ago
10 months ago

People

(Reporter: ladybenko, Assigned: jdescottes)

Tracking

(Blocks 1 bug)

unspecified
Firefox 62
Dependency tree / graph

Firefox Tracking Flags

(firefox62 fixed)

Details

Attachments

(1 attachment)

(Reporter)

Description

a year ago
At the moment, the dark theme isn't as readable (i.e. links, no workers screen, etc.) as the light theme for the application panel. We should fix this.
Comment hidden (mozreview-request)
(Assignee)

Comment 2

11 months ago
Current patch fixes the readability of links. Couldn't find a clear pattern for disabled links in devtools, but reduced opacity seemed to be used in some places (style editor, memory panel).

Temporarily re-added the disabled class on the debug link while we wait on Bug 1464354.
Assignee: nobody → jdescottes
Status: NEW → ASSIGNED
Depends on: 1464354
(Assignee)

Comment 4

11 months ago
Victoria: what is your opinion on disabled links? 

We used to grey them out, but we don't have a good theme variable for this. Here I used:
- --theme-highlight-blue (--blue-55 in light theme, #75BFFF in dark theme) when enabled 
- opacity: 0.5 when disabled
Flags: needinfo?(victoria)
(Reporter)

Comment 5

11 months ago
mozreview-review
Comment on attachment 8980530 [details]
Bug 1460209 - Use theme colors for links in application panel;

https://reviewboard.mozilla.org/r/246688/#review253306

Thanks, the new blue is definitely more readable! I just have a minor comment about some class, and it's simple enough that I don't think it needs further review

::: devtools/client/application/src/components/Worker.js:127
(Diff revision 1)
>        },
>          Strings.GetStringFromName("unregister"))
>        : null;
>  
> -    const debugLinkDisabled = this.isRunning() ? "" : "worker__debug-link--disabled";
> +    const debugLinkDisabled = this.isRunning() ? "" :
> +                                                 "worker__debug-link--disabled disabled";

We shouldn't need two classes here to set a link as disabled.

`.worker__debug-link--disabled` doesn't have any style rules associated, so I'd suggest to use just `.disabled` here.

The rationale is that we might have other disabled links in then panel, and that's why we have the `.disabled` class for links. If we had a preprocessor we could just have the `.disabled` styles as a mixin an import it in every component that needs disabled links –and thus reuse code– but it's not the case here.
Attachment #8980530 - Flags: review?(balbeza) → review+
(Assignee)

Updated

11 months ago
Duplicate of this bug: 1464354
Comment hidden (mozreview-request)

Comment 8

11 months ago
Pushed by jdescottes@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/a31957c18991
Use theme colors for links in application panel;r=ladybenko

Comment 9

11 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/a31957c18991
Status: ASSIGNED → RESOLVED
Last Resolved: 11 months ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 62
Hi Julian, thanks for the screenshots. This disabled link styling looks great! Very similar to the Photon guidelines for disabled buttons https://design.firefox.com/photon/components/buttons.html#behaviours
Flags: needinfo?(victoria)
(Assignee)

Comment 11

11 months ago
Thanks for the feedback and for linking to the photon styleguide, I should check it more regularly :)

Updated

10 months ago
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.