Fix light/dark mode based on browser default - #162
Conversation
|
🌐 Preview URL: https://pr-162.frcsoftware.pages.dev |
|
Waiting for #203 then we can work towards merging this pr in |
…into fix-light/dark-mode-based-on-browser-default
…into fix-light/dark-mode-based-on-browser-default
|
🌐 Preview URL: https://pr-162-frcsoftware.frcsoftware.workers.dev |
There was a problem hiding this comment.
🟡 Changes recommended
The new theme logic only reads prefers-color-scheme once at initialization and won’t follow OS/browser theme changes during a session when no explicit theme is stored.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Updates the Starlight header override to ensure the site’s initial theme defaults to the user’s browser/OS preferred color scheme (instead of always defaulting to dark), while still respecting an explicit user-chosen theme saved in localStorage.
Changes:
- Add an inline theme initializer that reads an explicit stored theme (if present) and otherwise derives theme from
prefers-color-scheme. - Persist theme only when the user explicitly toggles it, avoiding storing browser-derived defaults.
File summaries
| File | Description |
|---|---|
| src/starlightOverrides/Header.astro | Adds inline theme initialization + toggle persistence logic to default to browser/OS appearance. |
Review details
- Files reviewed: 1/1 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| const getPreferredTheme = () => | ||
| window.matchMedia('(prefers-color-scheme: light)').matches | ||
| ? 'light' | ||
| : 'dark'; | ||
|
|
||
| const init = () => { | ||
| const stored = getStoredTheme(); | ||
| const explicit = | ||
| stored === 'light' || stored === 'dark' ? stored : null; | ||
|
|
||
| applyTheme(explicit || getPreferredTheme()); | ||
|
|
||
| document.querySelectorAll('[data-theme-toggle]').forEach((btn) => { | ||
| btn.addEventListener('click', () => { | ||
| const current = | ||
| document.documentElement.dataset.theme || | ||
| getPreferredTheme(); | ||
| const next = current === 'dark' ? 'light' : 'dark'; | ||
| applyTheme(next, true); | ||
| }); | ||
| }); | ||
| }; |
2d442a1 to
ee9bafd
Compare
There was a problem hiding this comment.
🟡 Changes recommended
The new theme script delays initialization until DOMContentLoaded and uses a storage-null check that can prevent OS theme changes from being applied when storage contains an unexpected value.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 1/1 changed files
- Comments generated: 2
- Review effort level: Lite
| if ( | ||
| document.readyState === 'complete' || | ||
| document.readyState === 'interactive' | ||
| ) { | ||
| init(); | ||
| } else { | ||
| document.addEventListener('DOMContentLoaded', init); | ||
| } |
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
🔵 Needs a closer look
The new theme script conditionally registers the OS color-scheme listener, which can break “follow system” behavior after a user changes theme preferences during a session.
Review details
Suppressed comments (1)
src/starlightOverrides/Header.astro:272
- The OS/browser color-scheme change listener is only registered when there is no explicit theme at init time. If a user later clears their explicit choice (e.g., selects a “System/Auto” option that removes/changes the stored value), the page will not start following OS theme changes until a reload because no listener was attached.
if (!explicit) {
if (typeof colorScheme.addEventListener === 'function') {
colorScheme.addEventListener('change', onSchemeChange);
} else if (typeof colorScheme.addListener === 'function') {
colorScheme.addListener(onSchemeChange);
}
}
- Files reviewed: 1/1 changed files
- Comments generated: 0 new
- Review effort level: Lite
Description
Previously, the website theme would default to dark mode regardless of the user's browser preferences. This fixes the theme to default to the browser's appearance.
Closes #161.
Meta
Merge checklist: