Closed Bug 1460209 Opened 2 years ago Closed 2 years ago
Polish dark theme for the application panel
59 bytes, text/x-review-board-request
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.
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
Some screenshots of the current patch: https://docs.google.com/document/d/14v3oQEAy8-Dz_R9pjMe_ajj-DmVkFH1hxrsng3FFwbg/edit?usp=sharing
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 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+
Pushed by firstname.lastname@example.org: https://hg.mozilla.org/integration/autoland/rev/a31957c18991 Use theme colors for links in application panel;r=ladybenko
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
Thanks for the feedback and for linking to the photon styleguide, I should check it more regularly :)
You need to log in before you can comment on or make changes to this bug.