docs: add auto/system theme option to theme toggle#24519
Open
dvdksn wants to merge 6 commits intodocker:mainfrom
Open
docs: add auto/system theme option to theme toggle#24519dvdksn wants to merge 6 commits intodocker:mainfrom
dvdksn wants to merge 6 commits intodocker:mainfrom
Conversation
On every page load, theme.js was unconditionally writing the resolved preference to localStorage, permanently overriding the OS dark-mode setting after the first visit. Removed the unconditional write and dead commented-out code from theme.js; the script now only reads and applies the class. The header Alpine component now manages the full lifecycle with a three-state cycle (light -> dark -> auto): in auto mode it removes the localStorage key, resolves from prefers-color-scheme, and listens for live system preference changes via matchMedia. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
✅ Deploy Preview for docsdocker ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
dvdksn
added
agent/generated
status/review
Contributor
Author
|
/review |
- Use icons/contrast.svg (not "contrast") to avoid build failure — the short name resolves to contrast-fill.svg which doesn't exist - Remove eager applyTheme(theme) call on init to prevent double-apply and potential flicker (theme.js already set the class on page load) - Store mql change listener in variable and return cleanup function from x-init so Alpine can remove it if the component is destroyed - Add hidden attribute to moon and contrast spans as no-JS fallback so only the sun icon shows when Alpine fails to initialise Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Contributor
Author
|
/review |
![docker-agent[bot]](https://avatars.githubusercontent.com/in/2787757?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Assessment: 🟢 APPROVE
This PR successfully implements a three-state theme toggle (light → dark → auto) for the Docker documentation site. The changes properly handle:
- Theme initialization: The early
theme.jsscript correctly reads from localStorage and applies the theme before page render, preventing flash of unstyled content - Three-state cycling: The Alpine.js toggle cycles through light → dark → auto as intended
- System preference tracking: The
matchMedialistener properly updates the theme when the OS preference changes while in auto mode - Icon display: Uses Alpine.js
x-showdirectives to display the appropriate icon (sun/moon/contrast) for each state
The implementation follows standard patterns for theme management and properly integrates with Alpine.js reactivity. No bugs or issues were found in the changed code.
Contributor
|
⏭️ Review skipped — another review for this PR is already in progress (started 1182s ago). Only one review runs at a time to avoid duplicate comments. The in-progress review will post its results when complete. |
Reverts the icon path change from the previous commit. contrast-fill.svg does exist at node_modules/@material-symbols/svg-400/rounded/contrast-fill.svg and is correctly mounted to assets/icons/ by Hugo. The short name "contrast" resolves to icons/contrast-fill.svg as intended. The docker-agent review comment was incorrect. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Alpine x-show toggles inline style="display:none" but does not remove the HTML hidden attribute. With hidden present, the browser stylesheet's display:none from [hidden] persists even when x-show evaluates to true, so the moon and contrast icons never become visible. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Contributor
Author
|
/review |
Before Alpine initializes, all three icon spans (sun/moon/contrast) were
visible simultaneously because x-show only manages style="display:none"
and has no effect before Alpine runs.
The site already defines [x-cloak=""] { display: none !important } in
global.css and uses x-cloak throughout (sidebar, gordon-chat, dropdowns).
Adding x-cloak to all three spans hides them via CSS before Alpine loads;
Alpine removes x-cloak during initialization after evaluating x-show, so
only the correct icon becomes visible — no flash.
This replaces the earlier attempt to use the HTML hidden attribute, which
is incompatible with x-show: Alpine removes style="display:none" when
x-show is true, but never removes the hidden attribute, so the UA
stylesheet's [hidden]{display:none} kept the icons permanently hidden.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
x-cloak hid all three icons until Alpine initialized, causing a visible delay before the correct icon appeared. The original two-state toggle had no such issue because icon visibility was pure CSS (dark:hidden / dark:block) — theme.js sets the root class synchronously in <head> before first paint, so the right icon was always visible immediately. Fix: extend that same synchronous mechanism to three states. theme.js now also sets data-theme-preference on <html> (alongside the existing className set). CSS rules in global.css use that attribute to hide the two wrong icons before Alpine loads. x-show still handles all post-click updates (Alpine is already running by then, so no delay). applyTheme() keeps the data attribute in sync on every state change. Remove x-cloak from all three spans — it is no longer needed, and was the source of the jank. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
theme.jsunconditionally wrote the resolved preference tolocalStorageon every page load, permanently locking in a concrete"light"or"dark"value even for first-time visitors — making it impossible to track OS preference changes after any site visit. Removed the unconditionallocalStorage.setItemfromtheme.jsso the early script only reads (never writes); the Alpine.js toggle inheader.htmlnow cycles through three states (light → dark → auto), with "auto" removing thetheme-preferencekey and resolving fromprefers-color-schemewith a livematchMediachange listener so the theme updates immediately when the OS preference changes. A contrast icon indicates auto mode; all three icon spans are driven by Alpinex-showdirectives rather than pure CSS dark-mode classes.Verified: logic matches the standard three-state pattern;
prefers-color-schememedia query andmatchMediachange listener are the canonical browser APIs for system-preference tracking.Checked: no other files reference
theme-preferenceor the theme toggle; no CSS changes required.Closes #23177