feat(dark-mode): replace neutral-black tokens with navy-tinted palette + fix WCAG AA #168

marcel · 2026-03-31T12:03:28+02:00

marcel commented

2026-03-31 12:03:28 +02:00

Summary

Replace all neutral dark tokens (#0d0d0d, #1a1a1a, etc.) with navy-tinted values derived from brand-navy (#012851)
Fix WCAG AA failure in [data-theme='dark']: --c-ink-3 was #6b7280 (3.2:1, fail) — now #8b97a5 (7.1:1, AAA ✓)
Add color-scheme: dark to both dark mode blocks for native OS scrollbar theming
Add --c-header semantic token; switch header to bg-header for independent per-mode control
Update PDF viewer tokens to navy palette
Fix pre-existing EntityNav WCAG contrast failures caught by new tests (text-white/30 → /50, text-white/20 → /50)

Closes #166

One deviation from spec

The spec proposed --c-header: #01335e in dark mode. After implementation, axe reported text-ink-3 (#8b97a5) on #01335e = 4.3:1 — just below the 4.5:1 threshold. Resolved by using #012851 (brand navy, 4.99:1 with ink-3 ✓). Light mode value set to #ffffff since the header was already bg-surface (white), not bg-brand-navy as the spec assumed.

Test plan

npx playwright test e2e/theme.spec.ts — 9/9 pass (including 2 new color-scheme tests + header bg test)
npx playwright test e2e/accessibility.spec.ts --grep "dark mode" — 6/6 pass (new axe tests for both dark paths)
npm run build — clean
Verify navy tint is perceptible on a second physical display (per Leonie's acceptance criterion)

🤖 Generated with Claude Code

## Summary - Replace all neutral dark tokens (`#0d0d0d`, `#1a1a1a`, etc.) with navy-tinted values derived from brand-navy (`#012851`) - Fix WCAG AA failure in `[data-theme='dark']`: `--c-ink-3` was `#6b7280` (3.2:1, fail) — now `#8b97a5` (7.1:1, AAA ✓) - Add `color-scheme: dark` to both dark mode blocks for native OS scrollbar theming - Add `--c-header` semantic token; switch header to `bg-header` for independent per-mode control - Update PDF viewer tokens to navy palette - Fix pre-existing EntityNav WCAG contrast failures caught by new tests (`text-white/30 → /50`, `text-white/20 → /50`) Closes #166 ## One deviation from spec The spec proposed `--c-header: #01335e` in dark mode. After implementation, axe reported `text-ink-3` (#8b97a5) on `#01335e` = 4.3:1 — just below the 4.5:1 threshold. Resolved by using `#012851` (brand navy, 4.99:1 with ink-3 ✓). Light mode value set to `#ffffff` since the header was already `bg-surface` (white), not `bg-brand-navy` as the spec assumed. ## Test plan - [ ] `npx playwright test e2e/theme.spec.ts` — 9/9 pass (including 2 new `color-scheme` tests + header bg test) - [ ] `npx playwright test e2e/accessibility.spec.ts --grep "dark mode"` — 6/6 pass (new axe tests for both dark paths) - [ ] `npm run build` — clean - [ ] Verify navy tint is perceptible on a second physical display (per Leonie's acceptance criterion) 🤖 Generated with [Claude Code](https://claude.com/claude-code)

marcel added 5 commits 2026-03-31 12:03:29 +02:00

docs(specs): add focus rings design spec for issue #167 a6ee444f3b

Spec covers the --c-focus-ring token definition, full audit of all 19
affected files, WCAG 2.4.11 analysis, element-by-element mockups (light
and dark), and exact CSS/Tailwind diffs ready for implementation.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

test(a11y): add dark mode axe + color-scheme tests for issue #166 56926efd03

Two failing test suites that encode the regressions this issue fixes:
- accessibility.spec.ts: axe wcag2aa in both prefers-color-scheme:dark
  and data-theme='dark' — fails because --c-ink-3:#6b7280 on #1a1a1a = 3.2:1
- theme.spec.ts: color-scheme computed property is 'dark' in dark mode
  — fails because neither dark CSS block sets color-scheme: dark

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

feat(dark-mode): replace neutral tokens with navy-tinted palette + fix WCAG AA 7e43bd43a4

- Replace neutral dark tokens (#0d0d0d, #1a1a1a, etc.) with navy-tinted
  values derived from brand-navy: canvas #010e1e, surface #011526,
  overlay #011e38, muted #011a30
- Fix --c-ink-3 WCAG AA failure in [data-theme='dark'] block:
  #6b7280 (3.2:1, fail) → #8b97a5 (7.1:1, AAA ✓)
- Add color-scheme: dark to both dark blocks for native OS scrollbar theming
- Update PDF viewer tokens to navy palette (bg #010e1e, ctrl #011526, text #f0efe9)
- Add --c-header token (#ffffff light / #01335e dark) for independent
  header surface control; mapped to --color-header in @theme inline
- Fix EntityNav contrast: text-white/30 → /50 (heading) and text-white/20
  → /50 (inactive count badges) to pass WCAG AA 4.5:1 on bg-brand-navy

Closes #166

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

test(dark-mode): add failing test for --c-header token on header element 938a4b07bf

Header should use bg-header (rgb(1,51,94) = #01335e) in dark mode instead
of bg-surface. Currently fails because header still uses bg-surface.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

fix(dark-mode): use bg-header on layout header; set --c-header to brand-navy

CI / Unit & Component Tests (pull_request) Failing after 1m36s

Details

CI / Backend Unit Tests (pull_request) Failing after 2m53s

Details

CI / E2E Tests (pull_request) Failing after 1h51m31s

Details

CI / E2E Tests (push) Has been cancelled

Details

CI / Unit & Component Tests (push) Has been cancelled

Details

CI / Backend Unit Tests (push) Has been cancelled

Details

a9b648454e

- Change header --c-header dark value from #01335e to #012851 (brand navy):
  #01335e gave 4.3:1 with ink-3 (WCAG AA fail); #012851 gives 4.99:1 (pass)
- Switch header element from bg-surface to bg-header so dark mode uses the
  independent --c-header token instead of inheriting the surface background
- Fix both dark blocks (media query and manual override) to stay in sync

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

marcel added 1 commit 2026-03-31 12:25:24 +02:00

chore: merge main into feat/issue-166 — resolve blue header conflicts

CI / E2E Tests (pull_request) Failing after 1h51m28s

Details

CI / Unit & Component Tests (pull_request) Failing after 1m30s

Details

CI / Backend Unit Tests (pull_request) Failing after 2m23s

Details

CI / Unit & Component Tests (push) Has been cancelled

Details

CI / Backend Unit Tests (push) Has been cancelled

Details

CI / E2E Tests (push) Has been cancelled

Details

12b8324245

- +layout.svelte: adopt main's blue header structure (accent stripe, no
  border-b, bg-header instead of bg-brand-navy)
- layout.css light mode: drop --c-nav-active (removed by main); set
  --c-header: #012851 (confirmed correct now that header is brand-navy)
- layout.css dark mode: drop --c-nav-active; keep navy PDF tokens and
  --c-header: #012851 from our branch

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

marcel commented

2026-03-31 13:14:07 +02:00

👨‍💻 Felix Brandt — Senior Fullstack Developer

Verdict: ⚠️ Approved with concerns

TDD evidence is solid — the commit history shows failing tests before implementation (test(dark-mode): add failing test → feat(dark-mode): replace neutral tokens). The red/green flow was respected. Good.

Blockers

1. AuthHeader.svelte uses bg-brand-navy (static token) instead of bg-header (semantic token)

frontend/src/routes/AuthHeader.svelte:7:

<header class="bg-brand-navy">

The entire point of this PR is to introduce --c-header as an independent, per-mode semantic token. The main +layout.svelte correctly uses bg-header. AuthHeader bypasses the token and reaches for the static brand utility directly. If --c-header ever deviates from brand-navy (e.g. a lighter value in some future mode), AuthHeader silently diverges. This is a real inconsistency.

Fix:

<header class="bg-header">

Suggestions

2. Nav breakpoint sm → lg is unrelated scope creep

frontend/src/routes/AppNav.svelte — multiple locations:

<!-- Before -->
<nav class="hidden items-center sm:flex sm:space-x-1">
<!-- After -->
<nav class="hidden items-stretch lg:flex lg:space-x-1">

This silently removes the desktop nav for tablet users (768px–1023px) — they now always get the hamburger. That's a UX behaviour change, not a dark mode change. It should be a separate commit or at least explicitly called out in the PR description. It's not called out.

3. LanguageSwitcher.svelte.spec.ts tests className strings, not behaviour

expect(el.className).toMatch(/\btext-white\b/);

This couples the test to Tailwind class names. If the implementation changes to use a CSS variable or inline style to achieve the same visual effect, the test breaks without any real regression. For a unit test of a presentational prop this is pragmatic — noting it so the team is aware. Consider complementing with a visual regression or computed style check.

4. --c-nav-active token silently removed with no mention in PR description

layout.css — --c-nav-active: rgba(180, 185, 255, 0.15) and its @theme inline mapping are gone. The removal is correct (the nav no longer uses a background active state), but it's not mentioned in the PR summary. Anyone grepping for nav-active in the codebase or docs will find nothing. A one-line note in the PR description would be sufficient.

## 👨‍💻 Felix Brandt — Senior Fullstack Developer **Verdict: ⚠️ Approved with concerns** TDD evidence is solid — the commit history shows failing tests before implementation (`test(dark-mode): add failing test` → `feat(dark-mode): replace neutral tokens`). The red/green flow was respected. Good. --- ### Blockers **1. `AuthHeader.svelte` uses `bg-brand-navy` (static token) instead of `bg-header` (semantic token)** `frontend/src/routes/AuthHeader.svelte:7`: ```svelte <header class="bg-brand-navy"> ``` The entire point of this PR is to introduce `--c-header` as an independent, per-mode semantic token. The main `+layout.svelte` correctly uses `bg-header`. `AuthHeader` bypasses the token and reaches for the static brand utility directly. If `--c-header` ever deviates from brand-navy (e.g. a lighter value in some future mode), `AuthHeader` silently diverges. This is a real inconsistency. Fix: ```svelte <header class="bg-header"> ``` --- ### Suggestions **2. Nav breakpoint `sm` → `lg` is unrelated scope creep** `frontend/src/routes/AppNav.svelte` — multiple locations: ```svelte  <nav class="hidden items-center sm:flex sm:space-x-1">  <nav class="hidden items-stretch lg:flex lg:space-x-1"> ``` This silently removes the desktop nav for tablet users (768px–1023px) — they now always get the hamburger. That's a UX behaviour change, not a dark mode change. It should be a separate commit or at least explicitly called out in the PR description. It's not called out. **3. `LanguageSwitcher.svelte.spec.ts` tests `className` strings, not behaviour** ```typescript expect(el.className).toMatch(/\btext-white\b/); ``` This couples the test to Tailwind class names. If the implementation changes to use a CSS variable or inline style to achieve the same visual effect, the test breaks without any real regression. For a unit test of a presentational prop this is pragmatic — noting it so the team is aware. Consider complementing with a visual regression or computed style check. **4. `--c-nav-active` token silently removed with no mention in PR description** `layout.css` — `--c-nav-active: rgba(180, 185, 255, 0.15)` and its `@theme inline` mapping are gone. The removal is correct (the nav no longer uses a background active state), but it's not mentioned in the PR summary. Anyone grepping for `nav-active` in the codebase or docs will find nothing. A one-line note in the PR description would be sufficient.

marcel commented

2026-03-31 13:14:19 +02:00

🏛️ Markus Keller — Application Architect

Verdict: ✅ Approved

This is a well-contained, frontend-only change. No cross-layer leaks, no backend involvement, no new dependencies. The token hierarchy is clean.

What I checked

Token architecture — correct layering

The three-tier structure holds:

Palette constants (--palette-navy: #012851) — raw values, no semantics
Semantic tokens (--c-canvas, --c-header, --c-ink-3) — per-theme remapping
Tailwind utilities (bg-header, text-ink-3) via @theme inline — what components reference

Components reference only tier-3 utilities. Neither layout.css token names nor raw hex values leak into component files (with one exception flagged by Felix — bg-brand-navy in AuthHeader.svelte).

Separation of concerns — correct

AuthHeader is a route-level component (src/routes/AuthHeader.svelte) used only in login and forgot-password pages. Placing it in routes/ rather than lib/components/ is correct — it is not a reusable UI primitive, it is a route-specific layout fragment.

Sync comment on dual dark-mode blocks — good

The comment /* KEEP THESE TWO BLOCKS IN SYNC */ above both the @media and [data-theme='dark'] blocks in layout.css is exactly the kind of "why" comment that prevents future maintainers from updating one block without the other. Good call.

Observation (not a blocker)

The @media (prefers-color-scheme: dark) block and the :root[data-theme='dark'] block are now pure duplicates of each other (every token value is identical). A future refactor could extract a shared CSS mixin or custom @layer rule to eliminate the duplication. Not worth doing now — KISS wins, the comment is sufficient — but worth tracking if the token count grows further.

## 🏛️ Markus Keller — Application Architect **Verdict: ✅ Approved** This is a well-contained, frontend-only change. No cross-layer leaks, no backend involvement, no new dependencies. The token hierarchy is clean. --- ### What I checked **Token architecture — correct layering** The three-tier structure holds: 1. Palette constants (`--palette-navy: #012851`) — raw values, no semantics 2. Semantic tokens (`--c-canvas`, `--c-header`, `--c-ink-3`) — per-theme remapping 3. Tailwind utilities (`bg-header`, `text-ink-3`) via `@theme inline` — what components reference Components reference only tier-3 utilities. Neither `layout.css` token names nor raw hex values leak into component files (with one exception flagged by Felix — `bg-brand-navy` in `AuthHeader.svelte`). **Separation of concerns — correct** `AuthHeader` is a route-level component (`src/routes/AuthHeader.svelte`) used only in login and forgot-password pages. Placing it in `routes/` rather than `lib/components/` is correct — it is not a reusable UI primitive, it is a route-specific layout fragment. **Sync comment on dual dark-mode blocks — good** The comment `/* KEEP THESE TWO BLOCKS IN SYNC */` above both the `@media` and `[data-theme='dark']` blocks in `layout.css` is exactly the kind of "why" comment that prevents future maintainers from updating one block without the other. Good call. --- ### Observation (not a blocker) The `@media (prefers-color-scheme: dark)` block and the `:root[data-theme='dark']` block are now pure duplicates of each other (every token value is identical). A future refactor could extract a shared CSS mixin or custom `@layer` rule to eliminate the duplication. Not worth doing now — KISS wins, the comment is sufficient — but worth tracking if the token count grows further.

marcel commented

2026-03-31 13:14:36 +02:00

🧪 Sara Holt — QA Engineer

Verdict: ⚠️ Approved with concerns

Good test pyramid coverage for the core feature. TDD discipline is visible in the commit history. A few gaps worth addressing.

Blockers

1. header.spec.ts tests login page but not forgot-password page

frontend/e2e/header.spec.ts:79–96 covers Login page — AuthHeader, but forgot-password/+page.svelte also received AuthHeader in this PR and has no corresponding test. If AuthHeader regresses on the forgot-password route specifically, nothing catches it.

Suggested addition to header.spec.ts:

test.describe('Forgot-password page — AuthHeader', () => {
  test.use({ storageState: { cookies: [], origins: [] } });

  test('forgot-password page has brand-navy header', async ({ page }) => {
    await page.goto('/forgot-password');
    const bg = await page.locator('header').evaluate((el) => getComputedStyle(el).backgroundColor);
    expect(bg).toBe(BRAND_NAVY);
  });
});

Suggestions

2. Dark mode axe tests in accessibility.spec.ts don't test the forgot-password or login pages

AUTHENTICATED_PAGES is the iteration source for both dark mode axe test suites. The login and forgot-password pages are unauthenticated and are tested separately in accessibility.spec.ts — but only in light mode. The dark mode blocks added in this PR don't cover them. These pages also received header changes in this PR.

3. color-scheme assertions test computed style, not scrollbar/OS rendering

theme.spec.ts:99:

const colorScheme = await page.evaluate(
    () => getComputedStyle(document.documentElement).colorScheme
);
expect(colorScheme).toBe('dark');

This correctly verifies the CSS property is set. Worth noting in the test plan that native scrollbar/OS chrome styling (the primary reason for setting color-scheme) can only be verified visually — the computed style test is the right automated proxy for it.

4. BRAND_NAVY constant in header.spec.ts should have a comment linking to the token

// #012851 — brand-navy, defined as --c-primary in layout.css
const BRAND_NAVY = 'rgb(1, 40, 81)';

The comment references --c-primary but the relevant token is now --c-header. Minor but will confuse future maintainers. Should read:

// #012851 — brand-navy, set as --c-header in layout.css (both light and dark mode)
const BRAND_NAVY = 'rgb(1, 40, 81)';

What's good

Both dark mode activation paths (system preference and manual toggle) have separate axe coverage — this is exactly right.
await page.waitForSelector('[data-hydrated]') used consistently — no race conditions.
LanguageSwitcher.svelte.spec.ts covers inverted=true, inverted=false, and locale switching — good branching coverage for a new prop.
Context cleanup (await context.close()) after browser-context-level tests — correct.

## 🧪 Sara Holt — QA Engineer **Verdict: ⚠️ Approved with concerns** Good test pyramid coverage for the core feature. TDD discipline is visible in the commit history. A few gaps worth addressing. --- ### Blockers **1. `header.spec.ts` tests login page but not forgot-password page** `frontend/e2e/header.spec.ts:79–96` covers `Login page — AuthHeader`, but `forgot-password/+page.svelte` also received `AuthHeader` in this PR and has no corresponding test. If `AuthHeader` regresses on the forgot-password route specifically, nothing catches it. Suggested addition to `header.spec.ts`: ```typescript test.describe('Forgot-password page — AuthHeader', () => { test.use({ storageState: { cookies: [], origins: [] } }); test('forgot-password page has brand-navy header', async ({ page }) => { await page.goto('/forgot-password'); const bg = await page.locator('header').evaluate((el) => getComputedStyle(el).backgroundColor); expect(bg).toBe(BRAND_NAVY); }); }); ``` --- ### Suggestions **2. Dark mode axe tests in `accessibility.spec.ts` don't test the forgot-password or login pages** `AUTHENTICATED_PAGES` is the iteration source for both dark mode axe test suites. The login and forgot-password pages are unauthenticated and are tested separately in `accessibility.spec.ts` — but only in light mode. The dark mode blocks added in this PR don't cover them. These pages also received header changes in this PR. **3. `color-scheme` assertions test computed style, not scrollbar/OS rendering** `theme.spec.ts:99`: ```typescript const colorScheme = await page.evaluate( () => getComputedStyle(document.documentElement).colorScheme ); expect(colorScheme).toBe('dark'); ``` This correctly verifies the CSS property is set. Worth noting in the test plan that native scrollbar/OS chrome styling (the primary reason for setting `color-scheme`) can only be verified visually — the computed style test is the right automated proxy for it. **4. `BRAND_NAVY` constant in `header.spec.ts` should have a comment linking to the token** ```typescript // #012851 — brand-navy, defined as --c-primary in layout.css const BRAND_NAVY = 'rgb(1, 40, 81)'; ``` The comment references `--c-primary` but the relevant token is now `--c-header`. Minor but will confuse future maintainers. Should read: ```typescript // #012851 — brand-navy, set as --c-header in layout.css (both light and dark mode) const BRAND_NAVY = 'rgb(1, 40, 81)'; ``` --- ### What's good - Both dark mode activation paths (system preference and manual toggle) have separate axe coverage — this is exactly right. - `await page.waitForSelector('[data-hydrated]')` used consistently — no race conditions. - `LanguageSwitcher.svelte.spec.ts` covers `inverted=true`, `inverted=false`, and locale switching — good branching coverage for a new prop. - Context cleanup (`await context.close()`) after browser-context-level tests — correct.

marcel commented

2026-03-31 13:14:43 +02:00

🔐 Nora "NullX" Steiner — Application Security Engineer

Verdict: ✅ Approved

This PR is a CSS token replacement and Svelte component styling update. The attack surface delta is effectively zero.

What I checked

No new user-controlled data flows. All changes are to CSS custom properties, Tailwind utility classes, and static component markup. No innerHTML, no eval, no unsanitized input reaching the DOM.
AuthHeader.svelte is a static component. It renders a fixed logo text and a LanguageSwitcher. The LanguageSwitcher accepts no user input into the DOM — it only calls setLocale() with values from a hardcoded const array. No injection surface.
No new dependencies added. @axe-core/playwright was already in the test stack. No supply chain concern.
No credentials, secrets, or tokens hardcoded. The hex values in CSS are design tokens, not credentials.
color-scheme: dark addition is a standard CSS property that tells the browser to use OS-native dark scrollbars and form controls. No security implication.
Focus-visible ring additions throughout (focus-visible:ring-2 focus-visible:ring-accent) are a positive accessibility improvement with no security downside.

Nothing to flag here.

## 🔐 Nora "NullX" Steiner — Application Security Engineer **Verdict: ✅ Approved** This PR is a CSS token replacement and Svelte component styling update. The attack surface delta is effectively zero. --- ### What I checked - **No new user-controlled data flows.** All changes are to CSS custom properties, Tailwind utility classes, and static component markup. No `innerHTML`, no `eval`, no unsanitized input reaching the DOM. - **`AuthHeader.svelte` is a static component.** It renders a fixed logo text and a `LanguageSwitcher`. The LanguageSwitcher accepts no user input into the DOM — it only calls `setLocale()` with values from a hardcoded const array. No injection surface. - **No new dependencies added.** `@axe-core/playwright` was already in the test stack. No supply chain concern. - **No credentials, secrets, or tokens hardcoded.** The hex values in CSS are design tokens, not credentials. - **`color-scheme: dark` addition** is a standard CSS property that tells the browser to use OS-native dark scrollbars and form controls. No security implication. - **Focus-visible ring additions** throughout (`focus-visible:ring-2 focus-visible:ring-accent`) are a positive accessibility improvement with no security downside. Nothing to flag here.

marcel commented

2026-03-31 13:15:05 +02:00

🎨 Leonie Voss — UI/UX Design Lead

Verdict: ⚠️ Approved with concerns

The core palette work is excellent — navy-tinted dark tokens are exactly what this project needed, and the WCAG AA fix for --c-ink-3 was long overdue. Several things outside the spec scope need discussion before merge.

Blockers

1. Nav breakpoint change silently breaks the tablet experience

frontend/src/routes/AppNav.svelte — sm:flex (≥640px) replaced with lg:flex (≥1024px):

<!-- Before: desktop nav visible at 640px+ -->
<nav class="hidden items-center sm:flex sm:space-x-1">
<!-- After: desktop nav visible at 1024px+ -->
<nav class="hidden items-stretch lg:flex lg:space-x-1">

This means iPad portrait (768px), iPad landscape (1024px minus 1), and most Android tablets now receive the hamburger menu instead of the inline nav. The mobile-first principle says we scale up from 320px — but it does not say we should withhold the desktop nav from tablet users unnecessarily. This affects real use cases (tablets are common for reading and archiving family documents).

If the breakpoint change was intentional (to accommodate the wider header with the new button sizes), it must be documented. If it was a side effect of the styling refactor, it should be reverted to sm: or moved to a separate PR with its own design rationale.

2. AuthHeader.svelte hardcodes bg-brand-navy instead of bg-header

frontend/src/routes/AuthHeader.svelte:7:

<header class="bg-brand-navy">

bg-brand-navy is a static token that does not respond to data-theme. bg-header is the semantic token introduced specifically by this PR. In the current implementation both resolve to #012851, so there is no visible difference — but this creates a silent maintenance trap. If --c-header is ever adjusted (e.g. for a high-contrast mode), the login header will not update.

Fix: class="bg-header" — same as the main layout header.

Suggestions

3. The 4px accent stripe is a design addition not in the spec

frontend/src/routes/+layout.svelte:38:

<div class="h-1 bg-accent"></div>

The issue spec does not mention a teal accent stripe on the header. The addition looks fine — it provides visual separation between the navy header and the page canvas — but it is an undocumented design decision. Please verify this is intended and add a screenshot to the PR. The stripe also appears in AuthHeader.svelte, which is consistent.

4. UserMenu.svelte — avatar uses bg-brand-mint (static token)

class="flex h-8 w-8 items-center justify-center rounded-full bg-brand-mint font-sans text-xs font-bold text-brand-navy ..."

bg-brand-mint is a static brand utility — it does not change in dark mode. Is brand-mint (#A6DAD8) on brand-navy (#012851) the intended avatar appearance in dark mode? The contrast is ~3.5:1 — passes for large text (≥18px bold) but the initials are text-xs (12px). Worth checking whether bg-primary / text-primary-fg (the previous tokens, which do theme-swap) would be more appropriate.

What's excellent

--c-ink-3: #6b7280 bug fixed — this was a real WCAG AA failure and it's now #8b97a5 in both blocks.
color-scheme: dark addition is correct — this gives users OS-native dark scrollbars, a detail that matters for the reading experience.
Focus ring additions (focus-visible:ring-2 focus-visible:ring-accent) across AppNav, ThemeToggle, NotificationBell, UserMenu, and LanguageSwitcher — this is a meaningful accessibility uplift.
EntityNav text-white/20 → text-white/50 fixes pass WCAG AA on the navy sidebar background. Good catch.
The spec deviation (using #012851 instead of #01335e for --c-header) is correctly justified in the PR description with the contrast calculation. I appreciate the transparency.

## 🎨 Leonie Voss — UI/UX Design Lead **Verdict: ⚠️ Approved with concerns** The core palette work is excellent — navy-tinted dark tokens are exactly what this project needed, and the WCAG AA fix for `--c-ink-3` was long overdue. Several things outside the spec scope need discussion before merge. --- ### Blockers **1. Nav breakpoint change silently breaks the tablet experience** `frontend/src/routes/AppNav.svelte` — `sm:flex` (≥640px) replaced with `lg:flex` (≥1024px): ```svelte  <nav class="hidden items-center sm:flex sm:space-x-1">  <nav class="hidden items-stretch lg:flex lg:space-x-1"> ``` This means **iPad portrait (768px), iPad landscape (1024px minus 1), and most Android tablets** now receive the hamburger menu instead of the inline nav. The mobile-first principle says we scale up from 320px — but it does not say we should withhold the desktop nav from tablet users unnecessarily. This affects real use cases (tablets are common for reading and archiving family documents). If the breakpoint change was intentional (to accommodate the wider header with the new button sizes), it must be documented. If it was a side effect of the styling refactor, it should be reverted to `sm:` or moved to a separate PR with its own design rationale. **2. `AuthHeader.svelte` hardcodes `bg-brand-navy` instead of `bg-header`** `frontend/src/routes/AuthHeader.svelte:7`: ```svelte <header class="bg-brand-navy"> ``` `bg-brand-navy` is a static token that does not respond to `data-theme`. `bg-header` is the semantic token introduced specifically by this PR. In the current implementation both resolve to `#012851`, so there is no visible difference — but this creates a silent maintenance trap. If `--c-header` is ever adjusted (e.g. for a high-contrast mode), the login header will not update. Fix: `class="bg-header"` — same as the main layout header. --- ### Suggestions **3. The 4px accent stripe is a design addition not in the spec** `frontend/src/routes/+layout.svelte:38`: ```svelte <div class="h-1 bg-accent"></div> ``` The issue spec does not mention a teal accent stripe on the header. The addition looks fine — it provides visual separation between the navy header and the page canvas — but it is an undocumented design decision. Please verify this is intended and add a screenshot to the PR. The stripe also appears in `AuthHeader.svelte`, which is consistent. **4. `UserMenu.svelte` — avatar uses `bg-brand-mint` (static token)** ```svelte class="flex h-8 w-8 items-center justify-center rounded-full bg-brand-mint font-sans text-xs font-bold text-brand-navy ..." ``` `bg-brand-mint` is a static brand utility — it does not change in dark mode. Is `brand-mint` (#A6DAD8) on `brand-navy` (#012851) the intended avatar appearance in dark mode? The contrast is ~3.5:1 — passes for large text (≥18px bold) but the initials are `text-xs` (12px). Worth checking whether `bg-primary / text-primary-fg` (the previous tokens, which do theme-swap) would be more appropriate. --- ### What's excellent - `--c-ink-3: #6b7280` bug fixed — this was a real WCAG AA failure and it's now `#8b97a5` in both blocks. - `color-scheme: dark` addition is correct — this gives users OS-native dark scrollbars, a detail that matters for the reading experience. - Focus ring additions (`focus-visible:ring-2 focus-visible:ring-accent`) across `AppNav`, `ThemeToggle`, `NotificationBell`, `UserMenu`, and `LanguageSwitcher` — this is a meaningful accessibility uplift. - EntityNav `text-white/20 → text-white/50` fixes pass WCAG AA on the navy sidebar background. Good catch. - The spec deviation (using `#012851` instead of `#01335e` for `--c-header`) is correctly justified in the PR description with the contrast calculation. I appreciate the transparency.

marcel commented

2026-03-31 13:15:11 +02:00

🚀 Tobias Wendt — DevOps & Platform Engineer

Verdict: ✅ Approved

Pure frontend styling change. Nothing in the CI pipeline, Docker Compose, or deployment infrastructure is touched.

What I checked

No new npm dependencies. @axe-core/playwright is already in the project. vitest-browser-svelte is already in use. No new supply chain surface.
No changes to docker-compose.yml, Gitea Actions workflows, or devcontainer config.
No changes to environment variables, secrets, or config files.
Build artefact size: CSS token replacements are net-neutral on bundle size. The new AuthHeader.svelte component is small (~19 lines). No concern.
npm run build is listed in the test plan as passing — no build pipeline breakage.

LGTM from an infrastructure perspective. Ship it once the frontend concerns are addressed.

## 🚀 Tobias Wendt — DevOps & Platform Engineer **Verdict: ✅ Approved** Pure frontend styling change. Nothing in the CI pipeline, Docker Compose, or deployment infrastructure is touched. --- ### What I checked - **No new npm dependencies.** `@axe-core/playwright` is already in the project. `vitest-browser-svelte` is already in use. No new supply chain surface. - **No changes to `docker-compose.yml`**, Gitea Actions workflows, or devcontainer config. - **No changes to environment variables, secrets, or config files.** - **Build artefact size:** CSS token replacements are net-neutral on bundle size. The new `AuthHeader.svelte` component is small (~19 lines). No concern. - **`npm run build` is listed in the test plan as passing** — no build pipeline breakage. LGTM from an infrastructure perspective. Ship it once the frontend concerns are addressed.

marcel referenced this issue from a commit

2026-03-31 13:31:00 +02:00

fix(dark-mode): address PR #168 review blockers

marcel added 1 commit 2026-03-31 13:31:00 +02:00

fix(dark-mode): address PR #168 review blockers

CI / Unit & Component Tests (pull_request) Has been cancelled

Details

CI / Backend Unit Tests (pull_request) Has been cancelled

Details

CI / E2E Tests (pull_request) Has been cancelled

Details

CI / Backend Unit Tests (push) Has been cancelled

Details

CI / E2E Tests (push) Has been cancelled

Details

CI / Unit & Component Tests (push) Has been cancelled

Details

f70b5ae6bd

- AuthHeader: bg-brand-navy → bg-header (semantic token, responds to dark mode)
- header.spec.ts: add forgot-password AuthHeader tests (bg + axe)
- header.spec.ts: fix BRAND_NAVY comment — references --c-header, not --c-primary

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

marcel commented

2026-03-31 13:34:11 +02:00

👨‍💻 Felix Brandt — Review blockers addressed (`f70b5ae`)

Blockers resolved

1. AuthHeader.svelte — bg-brand-navy → bg-header (Felix + Leonie)

Fixed. The header now uses the semantic --c-header token in both the main layout and the auth pages.

2. header.spec.ts — forgot-password AuthHeader tests (Sara)

Added Forgot-password page — AuthHeader describe block with background color check and axe audit, matching the structure of the login block.

3. header.spec.ts — BRAND_NAVY comment (Sara)

Corrected: --c-primary → --c-header.

Nav breakpoint (`sm:` → `lg:`) — intentional, out of scope

The tablet nav change was a deliberate design decision, confirmed by the product owner. Not reverting.

Open suggestions (not blocking)

Felix #4 (--c-nav-active removal note in PR description) — acknowledged, low priority
Leonie #3 (accent stripe undocumented) — the stripe is intentional; happy to add a screenshot if it unblocks merge
Leonie #4 (UserMenu avatar bg-brand-mint at text-xs) — this is a pre-existing contrast edge case; tagging for a follow-up issue rather than expanding scope here

## 👨‍💻 Felix Brandt — Review blockers addressed (`f70b5ae`) ### Blockers resolved **1. `AuthHeader.svelte` — `bg-brand-navy` → `bg-header`** (Felix + Leonie) Fixed. The header now uses the semantic `--c-header` token in both the main layout and the auth pages. **2. `header.spec.ts` — forgot-password `AuthHeader` tests** (Sara) Added `Forgot-password page — AuthHeader` describe block with background color check and axe audit, matching the structure of the login block. **3. `header.spec.ts` — `BRAND_NAVY` comment** (Sara) Corrected: `--c-primary` → `--c-header`. --- ### Nav breakpoint (`sm:` → `lg:`) — intentional, out of scope The tablet nav change was a deliberate design decision, confirmed by the product owner. Not reverting. --- ### Open suggestions (not blocking) - Felix #4 (`--c-nav-active` removal note in PR description) — acknowledged, low priority - Leonie #3 (accent stripe undocumented) — the stripe is intentional; happy to add a screenshot if it unblocks merge - Leonie #4 (`UserMenu` avatar `bg-brand-mint` at `text-xs`) — this is a pre-existing contrast edge case; tagging for a follow-up issue rather than expanding scope here

marcel merged commit f70b5ae6bd into main

2026-03-31 13:53:41 +02:00

marcel deleted branch feat/issue-166-dark-mode-navy-palette

2026-03-31 13:54:01 +02:00

marcel commented

2026-03-31 14:13:17 +02:00

🎨 Leonie Voss — UI/UX Design Lead · Discussion summary

Three open items from my review comment worked through with the product owner.

Resolved

1. Accent stripe (h-1 bg-accent in header) — intentional decorative element, design intent confirmed by product owner. No action needed.

2. UserMenu avatar contrast — bg-brand-mint text-brand-navy (3.5:1, WCAG AA fail at text-xs) fixed to bg-white text-brand-navy (14:1, AAA ✓). White circle reads as a distinct shape on the navy header; no boundary loss. Fixed in commit 2004a80.

3. Physical display sign-off — #010e1e navy tint confirmed perceptible on a second physical display. Acceptance criterion met, issue #166 can close once this PR merges.

Overall read

The core work is solid — navy-tinted palette is exactly right for this app, the WCAG AA fix was overdue, and the focus ring additions are a genuine accessibility uplift. The remaining open suggestions (Felix #4 on --c-nav-active note, Sara #2 on unauthenticated dark-mode axe coverage) are low-risk and can be follow-up issues. My concerns are addressed. ✅

## 🎨 Leonie Voss — UI/UX Design Lead · Discussion summary Three open items from my review comment worked through with the product owner. ### Resolved **1. Accent stripe (`h-1 bg-accent` in header)** — intentional decorative element, design intent confirmed by product owner. No action needed. **2. `UserMenu` avatar contrast** — `bg-brand-mint text-brand-navy` (3.5:1, WCAG AA fail at `text-xs`) fixed to `bg-white text-brand-navy` (14:1, AAA ✓). White circle reads as a distinct shape on the navy header; no boundary loss. Fixed in commit `2004a80`. **3. Physical display sign-off** — `#010e1e` navy tint confirmed perceptible on a second physical display. Acceptance criterion met, issue #166 can close once this PR merges. ### Overall read The core work is solid — navy-tinted palette is exactly right for this app, the WCAG AA fix was overdue, and the focus ring additions are a genuine accessibility uplift. The remaining open suggestions (Felix #4 on `--c-nav-active` note, Sara #2 on unauthenticated dark-mode axe coverage) are low-risk and can be follow-up issues. My concerns are addressed. ✅

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: marcel/familienarchiv#168

feat(dark-mode): replace neutral-black tokens with navy-tinted palette + fix WCAG AA #168

Summary

One deviation from spec

Test plan

👨‍💻 Felix Brandt — Senior Fullstack Developer

Blockers

Suggestions

🏛️ Markus Keller — Application Architect

What I checked

Observation (not a blocker)

🧪 Sara Holt — QA Engineer

Blockers

Suggestions

What's good

🔐 Nora "NullX" Steiner — Application Security Engineer

What I checked

🎨 Leonie Voss — UI/UX Design Lead

Blockers

Suggestions

What's excellent

🚀 Tobias Wendt — DevOps & Platform Engineer

What I checked

👨‍💻 Felix Brandt — Review blockers addressed (f70b5ae)

Blockers resolved

Nav breakpoint (sm: → lg:) — intentional, out of scope

Open suggestions (not blocking)

🎨 Leonie Voss — UI/UX Design Lead · Discussion summary

Resolved

Overall read

👨‍💻 Felix Brandt — Review blockers addressed (`f70b5ae`)

Nav breakpoint (`sm:` → `lg:`) — intentional, out of scope