Belén [:ladybenko] Reporter
	Description • 6 years 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.

	Julian Descottes [:jdescottes] Assignee
	Comment 2 • 6 years 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.

	Julian Descottes [:jdescottes] Assignee
	Comment 3 • 6 years ago

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

	Julian Descottes [:jdescottes] Assignee
	Comment 4 • 6 years 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

	Belén [:ladybenko] Reporter
	Comment 5 • 6 years 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.

	Pulsebot
	Comment 8 • 6 years ago

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

	Noemi Erli[:noemi_erli]
	Comment 9 • 6 years ago
bugherder

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

	Victoria Wang [:victoria]
	Comment 10 • 6 years ago

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

	Julian Descottes [:jdescottes] Assignee
	Comment 11 • 6 years ago

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