-
Notifications
You must be signed in to change notification settings - Fork 56
fix(frontend): theme the surfaces that stayed light in dark mode #10295
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
Changes from 44 commits
774059d
14fbf99
7ebfc5b
0a06189
30efa6d
642bb5f
15c195e
763dec3
e8da3a3
6055b31
3566d0a
4aab39c
4ef12a7
38747ed
3122a61
946eb0c
abf1996
4739b1a
e217f33
b777823
198e6b2
b46cffd
d48d55f
c5cb03e
207326e
c3cca10
be0e7ee
ae77ce4
92258d2
c3b3e9b
d21affa
8eed77b
f8ac285
9f2afe8
313b133
79d891c
31c8014
de36a8a
30a8241
441db4a
7a1d36a
4091840
a5edf71
48906c0
489d809
96ea50b
f454ee7
d17a7b1
5f755ab
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1 @@ | ||
| Added an experimental dark theme: development deployments default to it, and a switch in the account menu (marked alpha) toggles between light and dark, gated by the INFRAHUB_EXPERIMENTAL_DARK_THEME setting | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. P2: Custom agent flagged. This release-note fragment omits the linked issue ID; rename it to Prompt for AI agents
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. the leading + is towncrier's orphan_prefix, set in pyproject.toml, and it is the supported way to mark a fragment with no linked issue. renaming to infp-46-dark-theme.added.md would drop that marker, and towncrier would then read infp-46 as an issue number and render a dead link through issue_format, which builds /issues/infp-46. most fragments in changelog/ use the + form. |
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,125 @@ | ||
| # Theming | ||
|
|
||
| > Part of: `dev/knowledge/frontend/` | Related: [Styling Guidelines](../../guidelines/frontend/styling.md) | ||
|
|
||
| How the light and dark themes work, and how to change them safely. The short version: every colour | ||
| the app paints should resolve through a semantic token defined once per theme in a single file, and | ||
| dark mode is nothing more than a `dark` class on the document element swapping those definitions. | ||
|
|
||
| ## Where colours live | ||
|
|
||
| `frontend/packages/ui/src/styles/theme.css` is the single source of truth. It has three parts, and | ||
| a colour change touches one, two, or three of them depending on the change: | ||
|
|
||
| | Block | What it holds | | ||
| |---|---| | ||
| | `:root { … }` | The light palette: one custom property per semantic token, plus `color-scheme: light` | | ||
| | `.dark { … }` | The dark palette: the **same property names** with dark values, plus `color-scheme: dark` | | ||
| | `@theme inline { … }` | The Tailwind bridge: `--color-<name>: var(--<name>)` lines that turn each token into utilities (`bg-<name>`, `text-<name>`, `ring-<name>`, …) | | ||
|
|
||
| ### Change a colour in dark only | ||
|
|
||
| Edit its value inside the `.dark` block. Nothing else — no component changes, no `dark:` variants, | ||
| no light-theme risk, because the light value in `:root` is untouched. | ||
|
|
||
| ### Change a colour in both themes | ||
|
|
||
| Edit the token's value in `:root` and in `.dark`. Every call site follows. | ||
|
|
||
| ### Add a new token | ||
|
|
||
| Three edits in `theme.css`: a light value in `:root`, a dark value in `.dark`, and a | ||
| `--color-<name>: var(--<name>);` line in `@theme inline`. Then use `bg-<name>` / `text-<name>` | ||
| etc. in components. Name the token for its **role** (`--active`, `--danger-surface`, `--content`), | ||
| never its colour — a token called `--indigo` cannot honestly hold anything else. | ||
|
|
||
| Paired tokens follow the `X` / `X-surface` convention (`--danger` / `--danger-surface`, | ||
| `--active` / `--active-surface`): the bare name is the foreground/stroke, `-surface` is the tinted | ||
| background behind it. | ||
|
|
||
| ## How dark mode switches on | ||
|
|
||
| The `dark` class on `document.documentElement` is the only switch. The primitives live in the | ||
| design system (`frontend/packages/ui/src/theme/`), so anything built on `@infrahub/ui` can read and | ||
| offer the theme; the application owns only the *policy* that decides it. Three things manage the | ||
| class: | ||
|
|
||
| 1. **The pre-paint script** in `frontend/app/index.html` — a blocking inline script in `<head>` | ||
| that applies the class before the first frame, from the `infrahub.theme.resolved` localStorage | ||
| mirror. It is deliberately outside the module graph (it must run before any bundle loads), so | ||
| the storage key is duplicated there verbatim — renaming the key means changing both files in | ||
| the same commit. | ||
| 2. **`ThemeProvider`** (`frontend/app/src/entities/config/ui/theme-provider.tsx`) — the policy. | ||
| Decides the real theme once config arrives: the `dark_theme` experimental flag gates whether | ||
| dark is offered at all, `infrahub.theme.choice` holds this browser's explicit choice, and the | ||
| resolved outcome is applied to the class and mirrored back to storage (`applyTheme` and the | ||
| storage helpers come from `@infrahub/ui`). It fills the design system's `ThemeContext`, which is | ||
| what makes `ThemeSwitchMenuItem` — the ready-made switch a menu can drop in — render and work. | ||
| An absent flag (backend predates it) counts as enabled under a Vite dev server only — see | ||
| `entities/config/domain/rules/can-offer-dark-theme.ts`. | ||
| 3. **`useResolvedTheme`** (from `@infrahub/ui`) — how components *read* the current theme: a | ||
| `useSyncExternalStore` subscription to the class via MutationObserver. Components never read | ||
| storage or config for this; the document element is the single source of truth. | ||
|
|
||
| The deployment gate is `INFRAHUB_EXPERIMENTAL_DARK_THEME`, passed through in | ||
| `development/docker-compose.yml` only (default `true` there). The root compose file deliberately | ||
| has no passthrough while the theme is alpha. | ||
|
|
||
| ## Content that carries its own colours | ||
|
|
||
| Three renderers bake colours into their output and cannot be themed by CSS tokens: | ||
|
|
||
| - **Mermaid diagrams** — themed through `mermaid.initialize({ theme })`, called from a small rehype | ||
| plugin sequenced before the rendering plugin | ||
| (`shared/components/editor/markdown/markdown-with-mermaid.tsx`). The rendering plugin's own | ||
| `mermaidConfig` option is silently ignored by its browser build; its documentation says to call | ||
| `initialize` manually, and the browser build renders against that same global config. Two traps | ||
| worth knowing: the `mermaid` version range must stay compatible with the one `mermaid-isomorphic` | ||
| declares (two instances in the tree would mean configuring the wrong one), and the call must live | ||
| *inside* the pipeline — a render-phase call is dropped by the React Compiler, and an effect races | ||
| the child's async processing. A diagram's own `%%{init}%%` directive still wins, by mermaid's own | ||
| precedence. | ||
| - **GraphiQL** — has its own theme; the sandbox page passes the app's resolved theme through | ||
| `forcedTheme` so it can never disagree with the app around it. | ||
| - **Schema-defined colours** (role badges, kind palettes, user-picked hex values) — data, not | ||
| style. Rendered as-is in both themes; out of scope for tokens. | ||
|
|
||
| ## When a `dark:` variant is acceptable | ||
|
|
||
| Almost never — a fixed palette class (`bg-white`, `bg-gray-50`) is a bug even when it *looks* fine | ||
| in light, and pairing it with a `dark:` override duplicates per call site what a token defines | ||
| once. The two legitimate exceptions, both from | ||
| [Styling Guidelines](../../guidelines/frontend/styling.md): | ||
|
|
||
| - No token can express the difference — swapping assets, dark-only effects (backdrop blur). | ||
| - Categorical ramps where the hue carries no meaning (the sidebar avatar colours): there is no | ||
| semantic name to give a token, and the ramp has a single definition site, so the duplication a | ||
| token prevents cannot arise. | ||
|
|
||
| ## Verifying a colour change | ||
|
|
||
| - **Contrast**: WCAG AA needs 4.5:1 for normal text, 3:1 for large text and UI parts. Measure | ||
| against the surface the element *actually sits on*, compositing translucent layers — a mid-ramp | ||
| shade that passes on one theme's background usually fails on the other's (that is why `--active` | ||
| holds `indigo-700` in light but `indigo-400` in dark). | ||
| - **Probing gotcha**: Tailwind only generates classes that appear in source. A class assembled | ||
| dynamically in a devtools probe (`bg-${hue}-400/15`) silently resolves to nothing and reads as | ||
| transparent — probe with the exact class strings the component ships. | ||
| - **Both themes, always**: toggle via the account-menu switch, or | ||
| `document.documentElement.classList.toggle("dark")` in the console. The light theme is the | ||
| shipped default; a dark fix must not move light pixels unless that is the intent. | ||
|
|
||
| ## Test coverage | ||
|
|
||
| | Concern | Test | | ||
| |---|---| | ||
| | Flag/choice resolution, retention across flag flips | `entities/config/ui/theme-provider.test.tsx`, `entities/config/domain/rules/can-offer-dark-theme.test.ts` | | ||
| | Reading the theme from the class | `shared/hooks/use-resolved-theme.test.tsx` | | ||
| | The switch in the account menu, alpha tag, gating | `entities/user-profile/ui/account-menu.test.tsx` | | ||
| | Mermaid renders in the active theme, reacts to a flip, author directive wins | `shared/components/editor/markdown/markdown-with-mermaid.test.tsx` (asserts the colours baked into the real SVG) | | ||
| | First-paint, persistence, flag-off journeys | `tests/e2e/theme.spec.ts` (Playwright, needs a stack) | | ||
| | Docs screenshots stay light | pinned in `tests/utils.ts` | | ||
|
|
||
| The design-system package has no test runner, so tests for its theme primitives are hosted in the | ||
| application suite. The pre-paint script itself is reachable only by the e2e suite — it sits outside | ||
| the module graph, so no vitest test can import it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
P2: Custom agent: Flag AI Slop and Fabricated Changes
This PR adds an externally visible config and
/api/configschema field despite claiming no API/schema or config/env changes. Update the PR scope/impact documentation to mentiondark_themeandINFRAHUB_EXPERIMENTAL_DARK_THEME.Prompt for AI agents
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
description updated, it now lists the config field and the compose passthrough under impact