Skip to content
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.

Sign up for GitHub

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

Jump to bottom

feat: respect user's preferred color scheme (dark mode repair) #3102

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

feat: respect user's preferred color scheme (dark mode repair) #3102

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
0b94209
feat: respect-users-preferred-color-scheme
Oct 9, 2024
858a6e4
fix: theme toggles wrong way after clearing `localStorage`
Oct 14, 2024
4a523ac
fix: theme button don't change on matcher change
Oct 14, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 10 additions & 2 deletions src/app/shared/components/theme-mode-toggle/theme-mode-toggle.component.html
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,12 @@
<button class="theme-mode-toggle">
<span class="material-icons" *ngIf="!isDarkMode" (click)="toggleThemeMode()"> light_mode </span>
<span class="material-icons" *ngIf="isDarkMode" (click)="toggleThemeMode()"> dark_mode </span>
<span class="material-icons" *ngIf="theme === 'dark'" (click)="toggleTheme()">
light_mode
</span>
<span
class="material-icons"
*ngIf="theme === 'light'"
(click)="toggleTheme()"
>
dark_mode
</span>
</button>
54 changes: 31 additions & 23 deletions src/app/shared/components/theme-mode-toggle/theme-mode-toggle.component.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,49 +1,57 @@
import { Component, Inject, OnInit } from '@angular/core';
import { DOCUMENT } from '@angular/common';
import { MediaMatcher } from '@angular/cdk/layout';
import { DOCUMENT } from '@angular/common';
import { ChangeDetectorRef, Component, Inject, OnInit } from '@angular/core';
import { StorageService } from '../../services/storage.service';

type Theme = 'light' | 'dark';

@Component({
selector: 'app-theme-mode-toggle',
templateUrl: './theme-mode-toggle.component.html',
styleUrls: ['./theme-mode-toggle.component.scss'],
})
export class ThemeModeToggleComponent implements OnInit {
isDarkMode: boolean;
theme: Theme;

constructor(
@Inject(DOCUMENT)
private readonly document: Document,
private readonly mediaMatcher: MediaMatcher,
private readonly storageService: StorageService,
private readonly changeDetector: ChangeDetectorRef,
) {}

ngOnInit() {
// 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);
Comment on lines -22 to -26
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't just uncommenting this code be enough?

Copy link
Author

@jewhyena jewhyena Oct 17, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

@jewhyena jewhyena Oct 18, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


const isDarkMode = this.getUserSettingsIsDarkMode();
this.setThemeMode(isDarkMode);
const darkSchemeMatcher = this.mediaMatcher.matchMedia(
'(prefers-color-scheme: dark)',
);

darkSchemeMatcher.onchange = ({ matches }) => {
if (!this.getStoredTheme()) this.setTheme(matches ? 'dark' : 'light');
};

const preferredScheme = darkSchemeMatcher.matches ? 'dark' : 'light';
const storedTheme = this.getStoredTheme();

this.setTheme(storedTheme ?? preferredScheme);
}

toggleThemeMode() {
const isDarkMode = !this.isDarkMode;
this.storageService.set('theme-mode', isDarkMode.toString());
this.setThemeMode(isDarkMode);
toggleTheme(skipStorage = false) {
const newTheme = this.theme === 'dark' ? 'light' : 'dark';
// NOTE: We should skip saving theme in storage when toggle is caused by matchMedia change event
// Otherwise, once saved, it'll no longer correspond to the system preferences,
// despite the user not touching the toggle button themselves
if (!skipStorage) this.storageService.set('theme', newTheme);
this.setTheme(newTheme);
}

private getUserSettingsIsDarkMode(): boolean {
return this.storageService.get('theme-mode') === 'true';
private getStoredTheme() {
return this.storageService.get('theme') as Theme | null;
}

private setThemeMode(isDarkMode: boolean) {
this.isDarkMode = isDarkMode;
this.document.documentElement.setAttribute(
'mode',
isDarkMode ? 'dark' : 'light',
);
private setTheme(theme: Theme) {
this.theme = theme;
this.document.documentElement.setAttribute('mode', theme);
this.changeDetector.detectChanges();
}
}