-
-
Notifications
You must be signed in to change notification settings - Fork 8.5k
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
refactor(v2): refactor color mode system to enable all possibilities #2939
Conversation
Deploy preview for docusaurus-2 ready! Built with commit 2637668 |
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.
Since v2 has active users, it would be good to throw a proper error on older configuration parameters.
var defaultDarkMode = ${defaultDarkMode}; | ||
var respectUserPreference = ${respectUserPreference}; |
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.
It would be better to more explicitly describe in the option itself that what exactly is this preference. respectUserDarkModeSystemPreference
(being elaborate here)
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.
https://drafts.csswg.org/mediaqueries-5/#prefers-color-scheme
I don't like the idea to have dark mode
in the attribute name. As it's specified, the preferred color scheme may be extended in the future to other values like "sepia" (wouldn't be surprised to see new values being created for color blind people in the future).
Also not fan of "system". It's not part of the spec where this preference is configured exactly. It can be your OS, it can be your browser, it can be a browser plugin...
if (storedTheme !== null) { | ||
setDataThemeAttribute(storedTheme); | ||
} | ||
else { |
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.
Need to run prettier here?
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.
as it's a string literal it would be hard to run prettier here :)
Yes, this is what I'd like to do. Didn't work on the docs and migration plan for now, as I want your opinion on the new API first. |
BTW, as mentioned, the colors might be extended to "sepia" and other values in the future. Not totally a fan of having a boolean What about |
defaultValue seems much better and future proof. Everything else looks good - I would personally call it something more explicit than |
thanks @EthanSK will update the PR soon. About |
@slorber Here are some suggestions: But actually, since prefer-color-scheme is a css standard , maybe it's best to incorporate that in the name: However, I do think it isn't a big deal to have system in the name, because as you can see here (https://developer.mozilla.org/en-US/docs/Web/CSS/@media/prefers-color-scheme), "...to detect if the user has requested the system use a light or dark color theme." They mention that it is a system thing. My favourite would probably be |
@EthanSK my reasoning for I think it's simpler to have a default value to false than to true, and why I prefer By default, should we ignore the user preference? I think we do, otherwise D2 site owners might develop their website with a single dark theme (their own user preference), and report that some of their uses are actually seeing the light theme. That can be annoying, in particular for sites having I think respecting the user preference here is:
What about:
|
@slorber I agree with you, the default should be to ignore the user preference. TBH I did use the word 'override' in all of my examples, but the main suggestions was the rest of the word, I should've made that clear :p I like I also don't know if it should be the past tense Dam this is hard xD I think at the end of the day, it's probably best to not make it too verbose, because the user is going to read the documentation for how to use it anyway, so |
Thanks for the feedback So, the final config might be:
|
New PR to track: #3012 |
Breaking change!
This is a proposal for now to validate the API/config.
I'll add documentation and clear user errors to make migration easier.
Motivation
Related to:
The current settings are a bit confusing and do not allow maximum flexibility for all users needs
Some things impossible with current api:
The new settings should allow all possible variations and be less confusing.
Test Plan
Preview and local tests