Fix the admin theme toggle - #3072
Conversation
🦋 Changeset detectedLatest commit: 7838e58 The changes in this PR will be included in the next version bump. This PR includes changesets to release 17 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
@emdash-cms/admin
@emdash-cms/auth
@emdash-cms/auth-atproto
@emdash-cms/blocks
@emdash-cms/cloudflare
@emdash-cms/contentful-to-portable-text
emdash
create-emdash
@emdash-cms/gutenberg-to-portable-text
@emdash-cms/plugin-cli
@emdash-cms/plugin-types
@emdash-cms/registry-client
@emdash-cms/registry-lexicons
@emdash-cms/registry-moderation
@emdash-cms/registry-verification
@emdash-cms/sandbox-workerd
@emdash-cms/x402
@emdash-cms/plugin-ai-moderation
@emdash-cms/plugin-atproto
@emdash-cms/plugin-audit-log
@emdash-cms/plugin-color
@emdash-cms/plugin-embeds
@emdash-cms/plugin-field-kit
@emdash-cms/plugin-forms
@emdash-cms/plugin-webhook-notifier
commit: |
There was a problem hiding this comment.
This PR fixes the admin theme toggle by replacing the three-state cycle with a direct light/dark toggle that removes the stored override whenever the chosen scheme matches the OS preference. That is the right fix for the reported problem (clicks that previously cycled through system could leave the visible scheme unchanged), it matches the accessible two-state model described in the link, and it keeps working within the existing ThemeProvider/useTheme interface.
I read the diff, the full changed components and tests, the package changeset, and verified the changes against AGENTS.md conventions. All user-facing strings are Lingui templates, no directional Tailwind classes were introduced, the changeset is accurate and proportionate for a patch, and the new focused tests cover the toggle semantics — system matching, explicit overrides, and localStorage removal. No logic errors, regressions, or convention violations. The PR is clean.
🚀 Deploying Preview to Cloudflare 🚀Preview URL: https://codex-admin-two-state-theme-toggle.try.emdashcms.com, https://codex-admin-two-state-theme-toggle-emdash-playground.emdash-cms.workers.dev (commit 7838e58)This URL reflects your latest Preview deploymentPreview Deployments by commit
|
|
/accept-baselines |
|
✅ Baselines accepted by @ascorbic and committed to this PR. |
There was a problem hiding this comment.
I re-read the current diff and traced the changed files against the admin theme code. This is a re-review: my previous review approved the PR and found no issues. I concentrated on whether the current state introduces any new problems.
Approach judgment: Replacing the three-state theme cycle with a two-state light/dark toggle is the right fix for the reported problem (clicks that cycled through system without changing the visible scheme). It matches the accessible model described in the PR and stays within the existing ThemeProvider/useTheme interface. Removing the localStorage override when the chosen scheme matches the OS preference is the correct way to resume following the system.
What I checked: ThemeProvider.tsx, ThemeToggle.tsx, Header.test.tsx, ThemeToggle.test.tsx, the changeset, and the visual regression snapshot diffs. I also verified cross-cutting conventions (Lingui wrappers, RTL-safe Tailwind, comment discipline, changeset quality, no logged-out routes affected, no SQL or auth changes).
- The
setTheme("system")path now removes the storage key, which is correct becausegetStoredTheme()treats a missing/invalid key as"system". ThemeTogglecorrectly computes the next scheme fromresolvedTheme, samples the OS preference at click time, and stores only when the choice differs from the system.- All user-facing strings are Lingui template literals.
- No directional Tailwind classes were added or removed.
- The focused tests cover system-matching removal, explicit overrides, and label changes.
- The 24 updated visual regression baselines correspond exactly to the 12 page × 2 locale screenshots where the header toggle icon changed from the old
Monitorto the resolvedSun/Moon; this is expected for the UI change.
I found no logic errors, regressions, convention violations, or unaddressed issues since the prior review.
Verdict: approve — the PR remains clean.
Overlapping PRsThis PR modifies files that are also changed by other open PRs:
This may cause merge conflicts or duplicated work. A maintainer will coordinate. |
What does this PR do?
Replaces the admin header's three-state theme cycle with a direct light/dark toggle. Every click changes the visible color scheme; when the selected scheme matches the operating system preference, the admin removes its stored override and resumes following the system.
The button shows the resolved sun or moon state and exposes its next action as its accessible label. The interaction follows the model described in Lea Verou's Dark mode toggles: two states are enough.
Related issue: Not applicable.
Type of change
Checklist
pnpm typecheckpassespnpm lintpassespnpm testpasses (or targeted tests for my change)pnpm formathas been runmessages.pochanges except in translation PRs — a workflow extracts catalogs on merge tomain.AI-generated code disclosure
Screenshots / test output
The light state follows the system preference with no stored override:
One click applies the dark override and changes the header control to a moon:
Focused browser component tests: 8 passed across
ThemeToggleandHeader.