1460209 - Polish dark theme for the application panel

Status: RESOLVED FIXED

(DevTools :: Application Panel, enhancement)

Description

[Belén [:ladybenko]]

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 1

[Julian Descottes [:jdescottes]]

Attachment: Bug 1460209 - Use theme colors for links in application panel;

Review commit: https://reviewboard.mozilla.org/r/246688/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/246688/

Comment 2

[Julian Descottes [:jdescottes]]

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.

Comment 3

[Julian Descottes [:jdescottes]]

Some screenshots of the current patch: https://docs.google.com/document/d/14v3oQEAy8-Dz_R9pjMe_ajj-DmVkFH1hxrsng3FFwbg/edit?usp=sharing

Comment 4

[Julian Descottes [:jdescottes]]

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

Comment 5

[Belén [:ladybenko]]

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.

Comment 7

[Julian Descottes [:jdescottes]]

Comment on attachment 8980530 [details]
Bug 1460209 - Use theme colors for links in application panel;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/246688/diff/1-2/

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a31957c18991
Use theme colors for links in application panel;r=ladybenko

Comment 9

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/a31957c18991

Comment 10

[Victoria Wang [:victoria]]

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

Comment 11

[Julian Descottes [:jdescottes]]

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