feat: add support for system color scheme #3167
Conversation
|
This pull request is being automatically deployed with Vercel (learn more). nusmods-website – ./website🔍 Inspect: https://vercel.com/nusmodifications/nusmods-website/ah4qvt0h1 nusmods-export – ./export🔍 Inspect: https://vercel.com/nusmodifications/nusmods-export/n688dqiii |
There was a problem hiding this comment.
This looks like it won't automatically update when the browser changes mode. Plus, having a reducer read data from outside feels like a violation of FP principles as the reducer is no longer pure.
Maybe we should implement this a different way. The Redux store should go back to being a store of the user's setting. We can do the theme switching in a new useIsDarkMode hook similar to this useBrowserTheme hook, which can read from the user's setting and listen to changes to the browser's dark mode. This can be easily done using our existing useMediaQuery hook.
AppShell can then just do const isDarkMode = useIsDarkMode().
website/src/types/settings.ts
Outdated
| export type Mode = 'LIGHT' | 'DARK'; | ||
| export type Mode = 'DEFAULT' | 'LIGHT' | 'DARK'; | ||
|
|
||
| export const DEFAULT_MODE: Mode = 'DEFAULT'; |
There was a problem hiding this comment.
Hmm, is there a better name for this variable? "Default mode" doesn't mean much out of context hahaha. I can't think of anything though
There was a problem hiding this comment.
What about DEFAULT_DISPLAY_MODE? Or OS_DEFAULT_DISPLAY_MODE? Was actually thinking of refactoring all the Mode names to DisplayMode, i.e. LIGHT_DISPLAY_MODE or DARK_DISPLAY_MODE etc, so that it sounds less generic.
There was a problem hiding this comment.
DisplayMode also sounds a bit vague though. I've looked up a bunch of potential alternatives:
DarkColorPreference, inspired by Electron'snativeTheme.shouldUseDarkColorsColorScheme, from the CSS media queryAppearance, from macOS's System Preferences paneUserInterfaceStyle, from Apple's APIsTheme, from Microsoft's APIs
Unfortunately most of them also sound vague. Some also sound similar to our existing Theme, which I guess is fine as I blame Theme for being even more vague -- we should probably rename or remove it instead.
I think DarkColorPreference is the clearest, as this var and type will only be used to store the user's preference. The useIsDarkMode hook can return a boolean based on the preference + the CSS media query.
There was a problem hiding this comment.
See global PR comment for what I've done to resolve this.
|
@taneliang Thanks for the review and sanity check! Agree on the reducer reading data from the outside being pretty iffy! I kinda know how to move on from here with your suggestions. I will slowly work on this over the next week. |
|
Btw we'll also need to display the segmented control better on
|
|
I think if we just change the label to "Auto" (which is still telling) it looks fine. 
This is on iPhone SE. |
ravern
force-pushed
the
os-default-night-dark-mode
branch
from
82242a5
to
dc49895
Compare
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
@ravern is attempting to deploy a commit to a Personal Account owned by @nusmodifications on Vercel. @nusmodifications first needs to authorize it. |
ravern
changed the title
ravern
force-pushed
the
os-default-night-dark-mode
branch
from
dc49895
to
d824b9b
Compare
|
I've rebased this onto the latest
The There was an issue where the behaviour of a Finally, regarding export, I refactored the export component to be a functional component instead, giving me access to |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #3167 +/- ##
==========================================
- Coverage 53.59% 53.53% -0.06%
==========================================
Files 272 274 +2
Lines 5988 6017 +29
Branches 1434 1443 +9
==========================================
+ Hits 3209 3221 +12
- Misses 2779 2796 +17 ☔ View full report in Codecov by Sentry. |
| export const DARK_MODE: Mode = 'DARK'; | ||
| /** | ||
| * Color schemes simply define whether the website is in light or dark mode. This | ||
| * is not to be confused with themese, which define the color palette of the website. |
| export type Theme = { | ||
| readonly id: ThemeId; | ||
| readonly name: string; | ||
| }; | ||
|
|
||
| export type Mode = 'LIGHT' | 'DARK'; |
There was a problem hiding this comment.
wow this is a much-needed refactoring LOL good stuff
| @@ -114,7 +114,7 @@ const SettingsContainer: React.FC<Props> = ({ | |||
| </p> | |||
There was a problem hiding this comment.
it might be good to add the fact that "Auto" uses your system preferences in this protip here^
Context
Very WIP, just the preliminary implementation first. PRing for sanity check.
Based on #1289.
Implementation
Roughly:
prefers-color-schememedia queryTodo
Note:
The keyboard toggling and setting of dark mode feels a bit laggy in dev, but isn't sluggish once exported. There is also incorrect snackbar notification text, which will be fixed in #3165.
Picture:
GIF:
