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.

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

feat: respond to the change of system theme appearance #877

Merged
merged 1 commit into from
Apr 2, 2024

Conversation

ambar
Copy link
Contributor

@ambar ambar commented Mar 28, 2024

Summary

Splits:

In this PR, all the appearance related codes are refactored to a single React Hook, the checklist:

  • Should respond to system preference change (light/dark/auto)
  • Should respond to local storage change (every tab should be in sync)
  • Should be in sync with Media Query (prefers-color-scheme)
  • Should not flash in dev/prod mode
  • Should not break the usage of window.RSPRESS_THEME, however, the use of global variables is flawed, see comments beblow #issuecomment-2027006896

After these are done, we can use two patterns to display elements without flash:

<!-- double element trick by class hook of Tailwind -->
<MyComponent className="dark:hidden" />
<MyComponent className="hidden dark:block" />

<!-- native CSS media query (image/video only) -->
<picture>
  <source srcset="dark.svg" media="(prefers-color-scheme: dark)" />
  <source srcset="light.svg" media="(prefers-color-scheme: light)" />
  <img src="light.svg" />
</picture>

Related Issue

Checklist

  • I have added changeset via pnpm run change.
  • I have updated the documentation.
  • I have added tests to cover my changes.

Copy link

netlify bot commented Mar 28, 2024

Deploy Preview for aquamarine-blini-95325f ready!

Name Link
🔨 Latest commit 476c859
🔍 Latest deploy log https://app.netlify.com/sites/aquamarine-blini-95325f/deploys/660b8f5bca556500089689bb
😎 Deploy Preview https://deploy-preview-877--aquamarine-blini-95325f.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.
Lighthouse
Lighthouse
1 paths audited
Performance: 91 (🟢 up 2 from production)
Accessibility: 97 (no change from production)
Best Practices: 100 (no change from production)
SEO: 92 (no change from production)
PWA: -
View the detailed breakdown and full score reports

To edit notification comments on pull requests, go to your Netlify site configuration.

@chenjiahan
Copy link
Member

Hi, thanks for your contribution. ❤️

Can you make separate PRs for each change? It will help us to review the changes.

@ambar
Copy link
Contributor Author

ambar commented Mar 28, 2024

@chenjiahan It's possible, but the changes are dependent on each other, I can take a try.

@ambar
Copy link
Contributor Author

ambar commented Mar 28, 2024

@chenjiahan You could review #878, #879 first

@10Derozan
Copy link
Contributor

Nice, and we can remove theme context. @sanyuan0704

@10Derozan
Copy link
Contributor

But, have you tested it, I think it still flash in production when enable ssg.

@ambar
Copy link
Contributor Author

ambar commented Mar 29, 2024

It's working in the Netlify Preview, even if the JavaScript is turned off, the images will not flash.
Some thoughts:

  • RSPRESS_THEME/MODERN_THEME could be removed (or add a option to theme config), as it cannot be reached in the inline theme script, this is a existing issue before this PR. If you're using it, you may see the theme flash after mount.
  • We could add a option to hardcode the theme to light or dark, this will replaces the darkMode: boolean.

@Timeless0911
Copy link
Contributor

replaces the darkMode: boolean seems to introduce some breaking change? How about expand the type of darkMode, maybe like this?

interface DarkModeSettings {
    toggleButton: boolean;
    defaultMode: "dark" | "light" | "auto";
}

type DarkModeConfig = boolean | DarkModeSettings;

darkMode?: DarkModeConfig;

Timeless0911
Timeless0911 previously approved these changes Apr 2, 2024
@Timeless0911
Copy link
Contributor

hi,#878 and #879 have been merged~ You can continue your separate

@ambar ambar changed the title feat: improve theme appearance feat: respond to the change of system theme appearance Apr 2, 2024
@ambar
Copy link
Contributor Author

ambar commented Apr 2, 2024

@Timeless0911 Thanks, rebased, this is the last PR, updated the description.

@sanyuan0704
Copy link
Contributor

Awesome

@sanyuan0704 sanyuan0704 merged commit 55cb0f4 into web-infra-dev:main Apr 2, 2024
11 checks passed
@chenjiahan chenjiahan mentioned this pull request Apr 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants