feat: respect user's preferred color scheme (dark mode repair) #3102
Conversation
|
I guess this isn't working on Ubuntu (firefox) 🤔 I tested the deploy preview demo_linux.webm |
Hm. That is strange. There's no platform-dependent code. It works seamlessly on both Chrome and Firefox on my Mac, as well as on my iPhone. By any chance, didn't you manually change the theme on the site before starting switching your system preference? Was your Because once you manually change the theme on the site, it no longer respects your system preference. After you click the button, the |
jewhyena
force-pushed
the
fix/respect-users-preferred-color-scheme
branch
from
be12438 to
858a6e4
Compare
|
still not working here 🤔 demo2.webmwhile the theme switching worked for discord desktop app |
Hmm. Since Discord is a different app which might not have any issues resolving the preferred schema on Ubuntu, let's check if it's the same with https://reactnative.dev/ and https://reactnavigation.org/ websites. Could you please tell me whether those two sites respect your color scheme or do nothing as well? |
|
they didn't work on firefox neither but I just found that everything went fine on chrome |
Might be some issue with GNOME: https://askubuntu.com/a/1464951. 🤔 |
| // This is commented out because by default the theme mode is set to light (at least for now) | ||
| // const userPrefersTheme = | ||
| // this.mediaMatcher.matchMedia && | ||
| // this.mediaMatcher.matchMedia('(prefers-color-scheme: light)').matches; | ||
| // this.setThemeMode(this.getUserSettingsIsDarkMode() || userPrefersTheme); |
There was a problem hiding this comment.
Wouldn't just uncommenting this code be enough?
There was a problem hiding this comment.
This is what I originally hoped for, but it didn't seem to be working (video attached). So I started fixing it.
Screen.Recording.2024-10-17.at.11.53.16.AM.mov
There was a problem hiding this comment.
Are you certain there's nothing in your local storage that specifies which mode to use? If I remember correctly, the commented-out code checks local storage first and, if it's empty, falls back to the user's preferred color scheme.
There was a problem hiding this comment.
Are you certain there's nothing in your local storage that specifies which mode to use?
Yes, absolutely. You can see in the video that my localStorage is empty.
Moreover, looking at the old code, it isn't even supposed to listen to any changes in color scheme preferences, since there is no onChange event listener specified anywhere. It only supposed to get the system preferences on the initial load (ngOnInit). But it turns out that it also doesn't work if we just uncomment the code. I get the light mode despite of my system preference being dark.
It might seem like the code on lines 28-29, which overrides everything returned by the mediaMatches, was put in there as a temporary substitution for the commented code. If we remove that and leave only uncommented code, it starts working but in the opposite direction: if my system preference is dark, it gives me the light mode; and, vice versa, if my system preference is light, only then it gives me the dark mode.
However, if we change the mediaMatcher query from (prefers-color-scheme: light) to (prefers-color-scheme: dark), it starts to give me the correct theme.
But it still does not listen to any changes in the system preferences. If you change your system preference manually, or if your OS configured to change the system preference automatically at different times of the day, you have to refresh your page in order for theme to change. This is what I intended to fix in this PR.
I added an event listener which—if there is no stored theme in localStorage—would automatically change the theme according to the system preference, without need to refresh the page. Also there is skipStorage parameter which allows skipping saving the theme in localStorage. We need this flag in order to prevent onChange event from storing anything in localStorage. Otherwise, once our system preference changes for the first time, the site will no longer respect it, despite the user not selecting the preferred theme manually by clicking the theme button. This is not the behavior we usually expect and I would consider it to be a bug.
What I'm not so sure about, though, is the use of changeDetectorRef. I don't know if this is a good or a bad practice, since this PR is basically the first time I ever touched Angular; so I would appreciate any advice on this.
The reason I have added this is that when the system preference change, the theme icon at the top right corner stays the same. You can notice it in the video I first attached to the PR. I recorded it before noticing it myself and before I pushed the fix.
For some reason, when theme change was caused by darkSchemeMatcher.onchange event, *ngIfs in the .html file didn't react to the change and stayed the same. So I found this approach with changeDetectorRef and with it all went as expected.
|
LGTM |
PR Checklist
Please check if your PR fulfills the following requirements:
PR Type
What kind of change does this PR introduce?
What is the current behavior?
Currently, the Nest.js documentation site does not respect the user's system preferences regarding the color scheme (light or dark). As a result, opening the site in a new browser or in an Incognito session always defaults to light mode, ignoring the system preferences.
Manually toggling the theme will save the current theme in
localStorage, so the next time the user opens the site, it will remain the same.Issue Number: N/A
What is the new behavior?
The new behavior respects the user's system preferences. If their preferred color scheme is dark, the Nest.js site will default to dark mode, and similarly, if their preferred color scheme is light, it will be in light mode. It now also listens for changes in the preferred color scheme, and the Nest.js site will automatically change its theme according to the current system preferences.
If the user manually toggles the theme button, the theme will be stored in
localStorageand remain there. In this case, the theme will not respect the system preferences. Instead,localStoragewill be the source of truth.Please see the attached video.
Does this PR introduce a breaking change?
Other information
Screen.Recording.2024-10-10.at.12.34.30.AM.mov