-
Notifications
You must be signed in to change notification settings - Fork 482
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
Respect user preference for color scheme #1456
Respect user preference for color scheme #1456
Conversation
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.
Currently it looks like it always defaults to the dark theme now, even though
> window.matchMedia('(prefers-color-scheme: dark)')
MediaQueryList {media: "(prefers-color-scheme: dark)", matches: false, onchange: null}
So it feels like something is broken here.
285dcdf
to
c7621e2
Compare
Tried to address all above comments, also confirmed now that it works as it is supposed to. |
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.
Awesome, seems to work nicely now! A few more things that popped up as I was playing around with this:
-
There is the
if(typeof(window.localStorage) === "undefined") return;
at the beginning ofset_theme_from_local_storage()
.This will mean that in browsers where
window.localStorage
isundefined
, but that prefer the dark theme, the theme will not be changed. So I think this should now be removed and the code below updated to make sure we handle the case where there is nolocalStorage
.I also discovered that
window.localStorage
can benull
(e.g. if you disablelocalStorage
). So I also think we should check withwindow.localStorage == null
instead. -
The theme picker in the settings dialog currently still shows "documenter-light" if the dark theme is selected by the browser. So we should update that code as well.
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 looks like I can push to this branch myself. Could you also bring the branch up to date with master and cherry-pick this CHANELOG commit: 2751b43.
Other than this and the one code suggestion, I think this is good to go 🙂
18dd2c3
to
6afa590
Compare
Could you merge master into this again? Or tick the "allow maintainers to push to this branch"? I can't merge this at the moment, because |
6afa590
to
50ad05a
Compare
Sorry, since this is created from the MLH fork, I wasn't able to do that. Although I've asked Cory to give you access to the MLH repos, so you should be able to push directly :) I've rebased the branch with master for now. |
Thanks @abhishalya! |
Automatically switches to 'non-primary' theme if user prefers dark scheme.
If there is a theme specified in the local storage then that takes more preference over this.
Closes #1320