-
Notifications
You must be signed in to change notification settings - Fork 1.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: support auto computing dark mode #1093
base: v6
Are you sure you want to change the base?
Conversation
@@ -117,7 +118,19 @@ class ZRender { | |||
? false | |||
: opts.useDirtyRect; | |||
|
|||
const painter = new painterCtors[rendererType](dom, storage, opts, id); | |||
const isDark = window.matchMedia && window.matchMedia('(prefers-color-scheme: dark)').matches; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to ensure the window exists. Add env.hasGlobalWindow
before window.matchMedia
and I recommend moving this line to 124 to avoid unnecessary invoking.
@@ -491,6 +504,8 @@ export interface ZRenderInitOpt { | |||
devicePixelRatio?: number | |||
width?: number | string // 10, 10px, 'auto' | |||
height?: number | string | |||
darkMode?: 'auto' | 'light' | 'dark' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Semantically, the darkMode
option sounds like a boolean value, do we need to support true/false
rather than 'dark'/'light'
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was intentionly using 'light'/'dark'
instead of boolean values because I think it may be ambiguous whether darkMode: true
means using dark mode whatever the system dark mode is, or use dark mode only when the system dark mode is true. I'm open to what you think. And to avoid making this looks like a boolean attribute, I didn't use useDarkMode
as useDirtyRect
or useCoarsePointer
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was intentionly using 'light'/'dark' instead of boolean values because I think it may be ambiguous whether darkMode: true means using dark mode whatever the system dark mode is, or use dark mode only when the system dark mode is true.
It looks clear enough to me:
- true - Always enable the dark mode.
- false - Never enable the dark mode even if the system is.
- 'auto' - follow the system preference.
This is how many apps do.
And if there is a must to avoid boolean value, 'colorScheme' (from CSS property color-scheme) may be a candidate for the name.
const painter = new painterCtors[rendererType](dom, storage, | ||
{ | ||
darkMode, | ||
...opts |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use extend
util instead of the extension operator.
Background
Currently, dark mode is a manually configured theme in Apache ECharts. It's troublesome to make another theme and making it consistant with the light theme as well as adjusting to make sure there is enough contrast between the foreground and the background.
In this PR, I propose a method to convert to dark mode without extra designing and developing.
Solution
The basic idea is to convert colors automatically into dark mode.
(TODO) I'm making more research on the algorithm, but for now, this PR simply inverse the lightness of the color and desaturate a little in dark mode. Even this simple algorithm works well enough.
Result
Here are some result of auto converted charts from the default Apache ECharts theme.
Discussion
Current implementation demostrates that auto converting algorithm is possible. We should probably improve the default light theme design (current theme doesn't has enough contrast in some cases like axis splitLine or uses too saturated colors in dataZoom) so that it looks well in both light and dark mode.
I'd suggest not supporting customizing dark theme for each options until many developers raised up such requirement after this feature releases, because otherwise it will bring in more complexity. In most cases, if developers want to customize the dark theme, they can opt-out the auto dark mode feature and set a customized dark theme as they do now.
TODO