-
Notifications
You must be signed in to change notification settings - Fork 56
Adapt remaining app to dark theme #10284
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
Open
bilalabbad
wants to merge
57
commits into
develop
Choose a base branch
from
bab-dark-theme-app
base: develop
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
57 commits
Select commit
Hold shift + click to select a range
0f2f1f8
refactor(frontend): adapt remaining app surfaces to dark theme
bilalabbad 3b3eb40
chore(frontend): refresh betterer results after dark theme sweep
bilalabbad f651eb9
fix(frontend): keep white text on the danger button in light theme
bilalabbad cc76ce1
review
bilalabbad 774059d
docs(specs): add dark theme completion specification
saltas888 14fbf99
docs(specs): add dark theme completion implementation plan
saltas888 7ebfc5b
docs(specs): critique dark theme completion and apply must-address fixes
saltas888 0a06189
docs(specs): add dark theme completion task breakdown
saltas888 30efa6d
docs(specs): add dark theme completion alignment check
saltas888 642bb5f
docs(specs): apply edge-case review to dark theme completion
saltas888 15c195e
docs(specs): keep dark opt-in; production defaults to light
saltas888 763dec3
docs(specs): gate dark theme behind an experimental flag
saltas888 e8da3a3
docs(specs): scope the flag to the dev compose file and name the defe…
saltas888 6055b31
feat(frontend): add theme resolution rule and resolved-theme type
saltas888 3566d0a
feat(frontend): bind GraphiQL and Mermaid to the active theme
saltas888 4aab39c
fix(frontend): tokenize always-light surfaces broken in dark mode
saltas888 4ef12a7
docs(specs): correct SC-004 after implementation found the real defects
saltas888 38747ed
fix(frontend): actually theme mermaid diagrams
saltas888 3122a61
docs(frontend): forbid fixed-palette surface classes
saltas888 946eb0c
fix(frontend): paint native browser UI in the active theme
saltas888 abf1996
refactor(frontend): drop a redundant dark variant from the gray badge
saltas888 4739b1a
fix(frontend): tokenize the active-state indigo
saltas888 e217f33
fix(frontend): give the avatar ramp and the template tile a dark palette
saltas888 b777823
feat(frontend): default non-production deployments to the dark theme
saltas888 198e6b2
feat(frontend): offer the dark theme when the backend predates the flag
saltas888 b46cffd
fix(frontend): paint reloads from the resolved mirror only
saltas888 d48d55f
fix(frontend): keep the mermaid directive out of quoted fences
saltas888 c5cb03e
fix(frontend): theme the alert close-button focus ring
saltas888 207326e
test(frontend): cover the theme's reading hook and its user journeys
saltas888 c3cca10
docs(dev): explain how theming works
saltas888 be0e7ee
docs(specs): reconcile spec artifacts with the implemented design
saltas888 ae77ce4
refactor(frontend): move theming into the design system, let mermaid …
saltas888 92258d2
fix(frontend): clear the contrast audit in both themes
saltas888 c3b3e9b
docs: document the dark theme for users
saltas888 d21affa
chore(frontend): refresh the betterer baseline after the theming move
saltas888 8eed77b
fix(docs): commit the configuration reference byte-exact with the gen…
saltas888 f8ac285
fix(ci): let the generator own the dark-theme flag in the root compos…
saltas888 9f2afe8
refactor(frontend): move the theme provider into the design system
saltas888 f95baab
bulk action in table dark theme
bilalabbad 313b133
chore: TEMPORARY — default the dark theme on so preview envs serve it
saltas888 79d891c
chore(spec): tick T059 now that the local CI gate has been run
saltas888 31c8014
Merge branch 'bab-dark-theme-app' into dark-theme-completion-infp-46
saltas888 de36a8a
fix(frontend): regenerate the REST types after the OpenAPI default ch…
saltas888 6a5b79e
color cleanup
bilalabbad 30a8241
Merge branch 'bab-dark-theme-app' into dark-theme-completion-infp-46
saltas888 441db4a
fix(frontend): theme the date-time picker
saltas888 7a1d36a
fix(frontend): theme the toasts
saltas888 4091840
fix(frontend): fade the IPAM header into its real surface, not into w…
saltas888 a5edf71
fix(frontend): sweep the remaining fixed-light surfaces
saltas888 48906c0
fix(frontend): retune GraphiQL's dark palette to the app's stone family
saltas888 489d809
fix: unbreak develop's tree E2E jobs and stop a class-name leak (#10287)
saltas888 96ea50b
fix(frontend): paint the theme before the first frame, not after it
saltas888 f454ee7
fix(frontend): tag only the option that switches into the alpha theme
saltas888 d17a7b1
revert: restore the dark theme's default to off
saltas888 5f755ab
feat(frontend): theme the schema visualizer with the application's theme
saltas888 498ce13
Merge pull request #10295 from opsmill/dark-theme-completion-infp-46
saltas888 b9bc6b4
Merge branch 'develop' into bab-dark-theme-app
saltas888 File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -990,6 +990,13 @@ class AnalyticsSettings(BaseSettings): | |
| class ExperimentalFeaturesSettings(BaseSettings): | ||
| model_config = SettingsConfigDict(env_prefix="INFRAHUB_EXPERIMENTAL_") | ||
| graphql_enums: bool = False | ||
| # Four generated artifacts record this default: the root compose file, the OpenAPI schema, the | ||
| # configuration reference, and the frontend REST types. The frontend one is regenerated from the | ||
| # OpenAPI schema rather than from this file, so it is the easy one to miss when this changes. | ||
| dark_theme: bool = Field( | ||
| default=False, | ||
| description="Offer the dark theme in the web interface. Alpha: some surfaces still render incorrectly.", | ||
| ) | ||
|
Comment on lines
+993
to
+999
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. Should this actually be a user preference instead of a global switch? |
||
| value_db_index: bool = Field( | ||
| default=False, | ||
| deprecated="This setting has no effect and will be removed in a future version.", | ||
|
|
||
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
| 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 |
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
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
| 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. |
Oops, something went wrong.
Oops, something went wrong.
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.
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.
this looks unnecessary