cockpit
cockpit copied to clipboard
cockpit-project
pkg: enable dark theme for PF
Both @jelly and I tried for a while today to get this to work by adding various files in different orders in different places... and it's not working right for either of us.
Perhaps there's some special magic to get it to work as expected that we're just not aware of at the moment?
We did also try to add in the following files in addition to those in this PR:
- @patternfly/patternfly/patternfly-theme-dark-prefers-color-scheme.scss
- @patternfly/patternfly/themes/dark/base.css
Nothing's working. Some widgets do show up a bit different (no shadows, darker borders) and sometimes we can get some to change (like white text on a white background when in dark mode), but it's nowhere near a dark mode yet. So we must be doing something wrong.
Mostly FYI:
- When
pf-theme-darkis set, patternfly enables the dark theme. - In chrom(ium) toggling dark mode can be done in devtools: click the three dotted (kebab) menu,
more tools->Rendering. Then select the emulation mode from "Emulate CSS media feature prefers-color-scheme"
Last rebase has imporved the situation some obvious issues:
- metrics page has issues
- top bar looks weird:
- Graphs are not readable (network manager page)
- Shell menu is hard to read
- Failing systemd unit
This uses --ct-color-list-critical-bg which needs some variant in pkg/lib/page.scss for our dark theme. This would be required for all our custom --ct-color-list values or at least have to be inspected.
Our loading animations are not looking ok
Oh, you didn't push your rebase... so, as it rebased cleanly, I just did. :wink:
One huge issue right now: On page load, the page is white. That's a dealbreaker for a proper dark theme. I guess we can set html or body to #000 on load (possibly even in the page, in a <style> block, if we can't get to it fast enough otherwise) and then let PF set it to whatever.
Oh, you didn't push your rebase... so, as it rebased cleanly, I just did. wink
Hmm strange, I force pushed again with a rebase plus fixes for some pages which did not have pf-dark-theme set.
The flashing is now gone, as we set the html background when creating the new iframe, but it feels that there is still some minor issue with flashing?
Unrelated:
networkmanager page does not have any difference in color between the header and the page itself?
Storage in contrast
I pushed up a bunch of fixes for dark mode. I'm still ignoring the issues that a newer PF would fix; these are ones that affect us specifically and ones that PF has not addressed.
While I did solve a few things already in Metrics, it still has white lines that I've been trying to combat. I wasn't able to figure it out in time, before heading off for a few days of PTO. I'll see you next week!
Rebased & stacked more fixes on top.
I couldn't find what I had done prior, but realized it was simpler than what I had done before, so I just fixed it here as-is.
But I think it's probably good enough for a beta now?! (We should probably reorder and squash a bit more and then merge this in.)
Text here seems unreadable.
Would this be fixed by updating our machines cockpit lib? (The PF overrides)
Talked with @garrett about pixel tests for dark theme. We think we could auto test dark theme on all biggest screenshots. We don't necessarily need medium and mobile for dark theme.
@martinpitt @jelly @mvollmer Any concerns/opinions about this?
Pixel tests will fail for the dark theme in the shell nav as the highlight color changed for the light theme:
the highlight color will still change, but i'll be an orangey yellow
Talked with @garrett about pixel tests for dark theme. We think we could auto test dark theme on all biggest screenshots. We don't necessarily need medium and mobile for dark theme.
@martinpitt @jelly @mvollmer Any concerns/opinions about this?
Yes, the question is how we can test this, I would not want to introduce a new test scenario:
- [x] does the CDP support setting dark theme?
- [ ] does Chrome allow setting via settings?
- [ ] does firefox allow it via settings?
Talked with @garrett about pixel tests for dark theme. We think we could auto test dark theme on all biggest screenshots. We don't necessarily need medium and mobile for dark theme. @martinpitt @jelly @mvollmer Any concerns/opinions about this?
Yes, the question is how we can test this, I would not want to introduce a new test scenario:
- [x] does the CDP support setting dark theme?
- [ ] does Chrome allow setting via settings?
- [ ] does firefox allow it via settings?
We can use:
https://chromedevtools.github.io/devtools-protocol/tot/Emulation/#method-setEmulatedMedia
Talked with @garrett about pixel tests for dark theme. We think we could auto test dark theme on all biggest screenshots. We don't necessarily need medium and mobile for dark theme. @martinpitt @jelly @mvollmer Any concerns/opinions about this?
Yes, the question is how we can test this, I would not want to introduce a new test scenario:
- [x] does the CDP support setting dark theme?
- [ ] does Chrome allow setting via settings?
- [ ] does firefox allow it via settings?
We can use:
https://chromedevtools.github.io/devtools-protocol/tot/Emulation/#method-setEmulatedMedia
Created a draft PR here https://github.com/cockpit-project/cockpit/pull/17854
I don't want to push more work on to this PR.
@garrett @marusak here are the new pixel tests:
New dark theme pixels https://cockpit-logs.us-east-1.linodeobjects.com/pull-17319-20221102-131822-0f1acac0-fedora-36/log.html
All looks good to me, this one has a small minor issue, maybe not even dark theme related
All looks good to me, this one has a small minor issue, maybe not even dark theme related
I think it's not dark theme related, but it's probably not seen in light mode... I think in light, it's a table with a white background on modal dialog with a white background.
But it's not a big deal. I think everything I've seen in the pixel tests are at the level of a cosmetic glitch and worthy of a follow-up. I'm fine with merging this, calling it "beta", and fixing whatever small bits we uncover (as well as these tiny things) after the release.
Everything seems functional and feature complete and nothing seems broken, in other words.
I think the release announcement needs a screenshot!
Good point!
We should probably take the screenshot using Cockpit Client, which has had dark mode for quite some time already. :wink:
I've included a heavily optimized GIF above. It's chonky, but will work everywhere and can be included with a standard img tag.
Here's a webm:
Here's an MP4 https://user-images.githubusercontent.com/10246/199786008-9178cc82-4fcc-49ff-bfd6-80ef4629087b.mp4
Original screenshots, if all else fails:
