datawrapper: Use dark background in dark mode #12831
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
At the moment, interactive block components set a white background in dark mode because interactive embeds are assumed not to handle dark mode. This was done in #10578, later modified in #10580 to remove transparency. This blocks the use of datawrapper’s automatic dark mode, because it causes it to display dark mode colours on a white background. This commit changes (only) datawrapper interactive block components to set a dark background in dark mode, which will let us use datawrapper’s automatic dark mode. I wasn’t entirely sure which colours to use: articleBackgroundDark seems the obvious choice (and is what I’ve gone with), but then why doesn’t interactiveBlockBackgroundList use articleBackgroundLight?
emdash-ie
force-pushed
the
support-datawrapper-auto-dark-mode
branch
from
6f6ae34 to
11ac343
Compare
|
Size Change: +67 B (+0.01%) Total Size: 940 kB ℹ️ View Unchanged
|
github-actions
bot
removed
the
run_chromatic
|
I’d like to test this on articles with other background colours in dark mode. From the definition of articleBackgroundDark it looks like this is dead blogs, live blogs, and any article when the theme is |
|
I’d also like to see it in the apps in dark mode. |
|
Another thing has occurred to me that I should test: what happens to Datawrapper graphs that don’t have auto dark mode enabled, in dark mode on this branch? (My worry is that we might get an unreadable graph using light-mode colours on a dark-mode background.) |
Thankfully, this is not as bad as I had feared: there’s still a white background as there currently is. (Not ideal, but no worse than it currently is.) I’ve added screenshots of this to the PR description. |
Since the change should only affect datawrapper graphics for which “auto dark mode” is enabled (and seeing specific articles from capi-CODE in the app seems like a big effort), I’m inclined to merge this before doing this check, and checking datawrapper graphics with “auto dark mode” enabled in the app in PROD (e.g. via the mystery bird article). |
I’ve tested live blogs and added screenshots to the PR description. Unfortunately the background colour is wrong and the graphic stands out from its background instead of blending in. Looks like the background is currently #000000 in this situation, but should be #1a1a1a. |
In this commit I’ve inlined articleBackgroundDark, and then changed `sourcePalette.neutral[0]` to `sourcePalette.neutral[10]` to match the background colour of the individual blocks in live blogs (rather than the article background).
domlander
approved these changes
Nov 14, 2024
There was a problem hiding this comment.
I wasn’t entirely sure which colours to use: articleBackgroundDark seems the obvious choice (and is what I’ve gone with), but then why doesn’t interactiveBlockBackgroundList use articleBackgroundLight?
I'm not sure what the answer to this is, but it looks like what you've done is sensible.
github-actions
bot
removed
the
run_chromatic
|
Looking at dead blogs, they use the same card colour as live blogs (#1a1a1a), but a different background (#121212 for dead instead of #000000 for live). Since these graphics will be on the cards, I should update this to use the same #1a1a1a background for dead blogs as for live. |
Dead blogs use the same card colour as live blogs (defined in liveBlockContainerBackgroundDark). This commit matches the datawrapper interactive dark mode background colour to this, to avoid clashing shades of dark grey.
Having these comments will hopefully make it clearer to anyone changing articleBackgroundDark that this colour needs to change as well.
I’ve made this update now in 3a2fffd. I’ll take some screenshots when I get my hands on CODE. |
|
Here is what the updated colours look like on live blogs. First with auto dark mode on: 
Then with auto dark mode off: 
I’ll update these in the table above too, and do new ones for dead blogs. |
|
Here are two screenshots showing the new colours on dead blogs:
Have also put these in the table in the PR description. |
github-actions
bot
removed
the
run_chromatic
|
@domlander Would you be ok to accept the pending changes in the UI tests? I’d like to have them accepted before merge if possible, and I don’t have the necessary access. Unfortunately the Datawrapper embeds are not loading the second time in the snapshots, so we only see the background in the dark mode embed. But these changes do show the background colour changing. It would be nice for the storybook to more accurately reflect the site, so that the background colours match and the different background colours are tested. I’ll take a look and see what I can do. |
Gonna take a look in a separate PR and merge this now, I reckon! |
|
Seen on PROD (merged by @emdash-ie 9 minutes and 51 seconds ago) Please check your changes! |
|
Auto dark mode looks good in the apps (testing in PROD with the mystery bird article now that the changes are out):
Though the text could maybe be a bit brighter, it’s a little low-contrast. |
I tested this in Android but not iOS, and it turns out that the font colour in iOS has not been responding to dark mode, resulting in black font disappearing against a black background. I'm looking into how to fix this fully, but in the mean time I'm going to revert this PR. |
emdash-ie
added a commit
that referenced
this pull request
Dec 16, 2024
…uto-dark-mode" It turns out the font colour on iOS is not responding correctly to dark mode graphics, giving us black text on a dark background. While we figure out how to fix that, we’d like to revert the support for auto-dark-mode, as adding it slightly reduced the spacing around light-mode graphics in dark mode. This reverts commit 90f72e9, reversing changes made to 89a9121.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What does this change?
This PR changes (only) datawrapper interactive block components to set a dark background in dark mode, instead of the white background currently set.
Why?
At the moment, interactive block components set a white background in dark mode because interactive embeds are assumed not to handle dark mode.
This was done in #10578, later modified in #10580 to remove transparency.
This blocks the use of datawrapper’s automatic dark mode, because it causes it to display dark mode colours on a white background. This PR lets us use the automatic dark mode.
Implementation Notes
I wasn’t entirely sure which colours to use: articleBackgroundDark seems the obvious choice (and is what I’ve gone with), but then why doesn’t interactiveBlockBackgroundList use articleBackgroundLight?
Screenshots
Note: the “after” is using a local instance of DCR with frontend-CODE, which seems to cause the overlap error with the sidebar behind.