Support dark mode #1888
Support dark mode #1888
Conversation
|
This will need a ton of work. There are way too many hard-coded colors in the current version of our CSS. They will need to be expressed in terms of CSS custom properties, and I have a hunch that there lurks also a really good opportunity to use not quite as many different colors, but to reduce by reusing fewer colors, which should also make the overall appearance nicer. However, I think I saw a couple of color definitions that were not even used. So before trying to encapsulate the color palette, we should probably use something like https://github.com/purifycss/purifycss to identify parts that are no longer needed (I manually identified three lines already that can easily go and stop bothering us). No need to adapt unused CSS. And finally, as I said elsewhere, I have no illusions about being a designer, and hope that someone with a proven record of a better design taste than myself (@To1ne maybe?) will give this some TLC. |
|
For shiggles, I branch-deployed this to https://dscho.github.io/git-scm.com/. |
Looking at that on Firefox 130.0.1 on Android it has some issues still. The dark mode just ends appruptly.
Switching to light mode only toggles the text, but not the background.
|
|
But I do think the parts that work look quite nice. |
Right, that's one thing I could not figure out. You will notice the
Thank you! There's still a ton of work to do, in particular cleaning up the CSS and then consolidating the remaining hard-coded colors into CSS custom properties à la |
dscho
force-pushed
the
dark-mode
branch
4 times, most recently
from
a5dc1e9 to
e0c17c7
Compare
|
I've decided to punt on the CSS clean-up for now ;-) @rimrul @To1ne I've updated this branch and deployed it to https://dscho.github.io/git-scm.com/; Would you mind giving it another go? For the record, here are a couple of Firefox Mobile screenshots: Front page
About - Branching and Merging
Sidebar
Documentation
A manual page
Versions drop-down of a manual page
Book, chapter 1 section 1
DownloadsHere, I present both the dark mode and the light mode to demonstrate that the monitor is cut off in both, therefore that's a bug that was not introduced by this PR:
|
|
@dscho I've had a more thorough look at it. Overall I'm quite impressed! Shout outs:
Few things:
All this is based on visual review, I didn't review any of the code. I can, if you like to? If you like I can submit some patches with suggestions? |
I would love that! |
Maybe next week. |
|
I just noticed that the selected OS in https://dscho.github.io/git-scm.com/downloads/guis?os=windows is not visibly discernible from non-selected ones in Dark Mode; That's also something that still needs to be fixed (leaving this here as a reminder). |
|
Just one problem that I found: some icons still have a bright background.
Other than that, wonderful! This will be quite nice to have. |
|
@TheGiraffe3 could you help out and contribute fixed icons, preferably as a PR to my branch? |
|
I'll try. |
|
There's a pull request up for that: dscho#16. |
|
@TheGiraffe3 Cool work. Do you happen to have screenshots of both in light and dark mode? @dscho About the logos in general. I think we should reconsider which/if they are relevant, but that's something for a separate MR/issue. Shall we open an issue on that? Or ask on the mailing list? My idea was to simply put the light background on all logos in CSS, even in dark mode (so the images without transparent background wouldn't stand out against the rest), but I haven't gotten to that yet. Then when we know which logos we want to keep/add/remove we could update the images accordingly. But thanks to @TheGiraffe3 that's not needed no more. |
On the website? |
@TheGiraffe3 doesn't need to be a deployed version, can be a screenshot when running the site locally. I suggest you have a look at the README and let us know if anything isn't clear how to preview the site locally? Anyhow, I've taken the liberty of running your changes locally: Light:
Dark:
Just as reference, this is the current original:
And this is the dark version @dscho 's branch:
|
|
I'll just note here that the old Eclipse logo was out of date by two versions and the background proved hard to remove, so we switched the logo out when changing it anyway. |
I should probably clarify that the "middle version" was an April Fools' joke:
In any case, here is the comparison after merging @TheGiraffe3's excellent work (thank you so much!): Light mode:
Dark mode:
|
|
The Microsoft logo in dark mode has been mangled quite a bit |
Hmm. I'm not even sure that this is a legitimate use of the trademark, see https://www.microsoft.com/en-us/legal/intellectualproperty/Trademarks/... |
To be precise, I am concerned about this guideline: Don’t imply an affiliation, endorsement, sponsorship, or approval with or by Microsoft. |
These images simply have too-dark labels otherwise. The proper solution would likely involve a separate set of images in that section, specifically targeting dark mode, with a specially-designed color palette. But I am not the person you want to do that, there are experts for this kind of stuff out there in the world, I ain't one of them. Signed-off-by: Johannes Schindelin <[email protected]>
They would otherwise stick out, brightness-wise. Signed-off-by: Johannes Schindelin <[email protected]>
This logo differs only in the colors (and in the format: it's an SVG), designed to work better with dark mode. Signed-off-by: Johannes Schindelin <[email protected]>
In dark mode these logos didn't look good. Refresh them. Signed-off-by: Toon Claes <[email protected]>
Image updates
To make it easier to style in dark mode. Signed-off-by: Toon Claes <[email protected]>
The image on the /about/data-assurance page looked creepy with the color filter. And because the image isn't bright anyway, remove the filter from that image. Signed-off-by: Toon Claes <[email protected]>
Signed-off-by: Toon Claes <[email protected]>
The icon to toggle between dark and light mode was added recently and this made laying out the elements in the header hard. A lot of items in the header have been using `position: absolute`. This was probably caused by the absolute positioning of the search result dropdown. Transition most elements in the header to use flexbox to make positioning more fluid and require less manual pixel pushing. Signed-off-by: Toon Claes <[email protected]>
Make the tagline scale nicely on every screen size. Signed-off-by: Toon Claes <[email protected]>
There was some js sourcery happening to make a click on the "Show all results..." entry in the search results dropdown go to the search page. Instead of doing the sourcery on click, set a proper `href` attribute on the `<a>`. This also makes browsers show a hand cursor when hovering the entry in the dropdown. Signed-off-by: Toon Claes <[email protected]>
Because it looks better to have the whole width for the search bar on mobile, move the dark-mode icon above the search bar, next to the tagline. To achieve this, use CSS grid where we can name the grid-areas and reposition them within media queries. Signed-off-by: Toon Claes <[email protected]>
Dark mode tweaks
This adds a responsive header, and other nice improvements. Signed-off-by: Johannes Schindelin <[email protected]>
css: place dark-mode icon next to tagline on mobile
In dark mode, those shadows simply do not look good. Signed-off-by: Johannes Schindelin <[email protected]>
I would have preferred a way to test this in a more generic way, e.g. by
getting the pixel value of the blurred image (to emulate averaging). I
was thinking about something like this:
const { w, h } = await page.evaluate(() => {
document.querySelector('#tagline').innerHTML = '--dark-mode-for-dark-times';
const overlay = document.createElement('div');
overlay.style.backdropFilter = 'blur(500px)';
overlay.style.backgroundColor = '#7f7f7f00';
overlay.style.width = '100%';
overlay.style.height = '100%';
overlay.style.position = 'absolute';
overlay.style.top = '0';
overlay.style.left = '0';
overlay.style.zIndex = '+5000';
document.body.insertBefore(overlay, document.body.firstChild);
return { w: window.innerWidth, h: window.innerHeight };
})
const pixel = await screenshot({ clip: { x: 50, y: 50, width: 1, height: 1 } });
I wanted to compare this pixel value between light/dark mode and ensure
that the light mode's pixel value is lighter than the dark mode one.
However, this approach did not work because `pixel`'s value is a
`Buffer` that contains a PNG, therefore I would have to jump through
hoops to get to that pixel value.
Besides, in my tests the screenshot was always taken so fast that the
renderer did not get a chance to render the blurred page before the
screenshot was taken. There is probably a way to listen for an event to
await that, but it all got too complicated for my liking.
In the end I settled for recording screenshots for all the browsers
supported by Playwright, at least for now.
The big advantage of using screenshots is that it is much clearer what
is being tested. A secondary advantage is that having individual
screenshots for the different browsers documents their (subtle)
differences.
Signed-off-by: Johannes Schindelin <[email protected]>
Hah, I forgot about this. The problem here is that I designed the "dark mode" settings to override the light-mode settings, but in dark mode, it became the default and the light-mode toggle could not override that. I fixed that like this: diff --git a/assets/sass/dark-mode.css b/assets/sass/dark-mode.css
index af36be6cc..fc51f2d76 100644
--- a/assets/sass/dark-mode.css
+++ b/assets/sass/dark-mode.css
@@ -184,6 +184,6 @@
@include mode($mode: dark, $theme: '[data-theme="dark"]')
@media screen and (prefers-color-scheme: dark) {
- @include mode($mode: dark)
- @include mode($mode: light, $theme: '[data-theme="light"]')
+ @include mode($mode: dark, $theme: ':not([data-theme="light"])')
+ @include mode($mode: light)
}
Done: 9675370 |
|
@To1ne do you think this is ready to merge? I kind of want to, and then take it from there. |
@dscho I think we are. I'm kind of tired working in a fork on this. One question though: Do we have/need a rollback strategy if we learn these changes were not ready to be published? I don't think it's needed, like what's the worst that can happen? And we can always do |
Yes, there is a "Revert" button once this PR is merged. And with that, I'll merge! |
Changes
This adds support for dark mode.
Context
It went through quite a few iterations, with contributions by @rimrul, @TheGiraffe3, and @To1ne, as well as a few other people who provided feedback to me privately.