feat(header): replace white header with brand-navy header per design spec #165
Reference in New Issue
Block a user
Delete Branch "feat/issue-163-brand-navy-header"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Closes #163
Summary
bg-brand-navy(#012851) — static token, stays navy in both light and dark modebg-accentstrip added above the nav barborder-b-2 border-accentbottom underlinetext-white/55, hovertext-white/85bg-brand-mint text-brand-navy(8.5:1 contrast)--c-nav-activeCSS token deleted; mobile drawer active state usesbg-accent-bgAuthHeader.sveltecomponent added for auth pages (logo + language switcher on navy bar)AuthHeaderinstead of the floating language switcherfocus-visible:ring-2 ring-accent) on all interactive header elementsLanguageSwitchergains aninvertedboolean prop for white text in dark-header contextsTest plan
#012851in light mode#012851after switching to dark mode (toggle theme toggle)e2e/header.spec.tspasses (4 tests)🤖 Generated with Claude Code
👨💻 Felix Brandt — Senior Fullstack Developer
Verdict: ⚠️ Approved with concerns
Blockers
AuthHeader.svelteduplicatesLanguageSwitcherinternals — violates DRYAuthHeader.sveltecontains its own locale list,localeMap,$derived(getLocale()), and button rendering logic — the exact same logic already encapsulated inLanguageSwitcher.svelte. The whole point of adding theinvertedprop toLanguageSwitcherwas to make it reusable on dark backgrounds.AuthHeadershould use it:This would also mean
AuthHeader.svelte's<script>block disappears entirely, making it a clean presentational component.The duplication meets the "3+ places" extraction threshold: the locale buttons now exist in
LanguageSwitcher.svelte,AuthHeader.svelte, and previously inlogin/+page.svelte(now removed). But we're still left with two.Suggestions
{#each}key inAuthHeader.svelteis good —(locale)is correct for a primitive list. ✓my-2+border-b-2active state may float above the header edge. The nav container usesitems-stretch, but each link hasmy-2(8px vertical margin), so theborder-b-2active underline sits 8px above the header's bottom edge. The visual spec may intend the underline to reach the very bottom of the bar. Worth verifying against the design.Test hygiene —
[data-hydrated]ties tests to implementation detail. The tests useawait page.waitForSelector('[data-hydrated]')as a hydration guard. If the attribute is ever renamed, two tests break silently on a non-UI change. Consider a role-based selector or a visible nav element as the readiness signal instead:TDD evidence: Tests are committed on the same branch alongside implementation — I'd prefer to see them in a prior commit establishing the failing state, but the coverage is there and meaningful. ✓
🏛️ Markus Keller — Application Architect
Verdict: ✅ Approved
This is a contained, frontend-only change. No new layers, no new dependencies, no backend or database involvement. Architecture is clean.
Observations
Removal of
--color-nav-active/--c-nav-activeis clean. The token is deleted from all three theme definitions (:root,[data-theme="dark"], and the second dark block). No zombie CSS variables left dangling. ✓AuthHeader.svelteplacement inroutes/vslib/components/: The component is placed next to the routes that use it, which is reasonable for a layout component used only by auth pages. However, since+layout.sveltealso lives inroutes/and imports from it, there's no layering violation. If the component ever needs to be used outside auth pages, move it tolib/components/.Breakpoint shift from
sm→lgfor desktop nav: The hamburger menu now appears up to 1023px instead of up to 639px. This means tablets (768px–1023px) always get the mobile drawer instead of the inline nav. This is a design choice and the implementation is consistent throughout (AppNav, the mobile overlayfixed inset-0 top-[68px] z-40 lg:hidden). No inconsistent breakpoints found. ✓The LanguageSwitcher duplication in AuthHeader (flagged by Felix) is a modularity concern. The component boundary exists precisely to avoid this. I'd want to see
<LanguageSwitcher inverted />used there before merge.No structural or layering violations. Frontend module boundaries respected.
🧪 Sara Holt — QA Engineer
Verdict: ⚠️ Approved with concerns
Good that tests exist — four E2E scenarios covering the most critical regression risks. Some gaps to address.
Blockers
No accessibility checks in E2E tests. My standard is
checkA11yon every critical page. The header is a global navigation landmark — contrast failures, missing ARIA labels, and focus trap issues all surface here. This is especially important given the contrast concerns Leonie will raise abouttext-white/55.Suggestions
[data-hydrated]is an implementation coupling. If this attribute is renamed or removed, the two hydration-dependent tests will time out silently rather than failing descriptively. A safer readiness signal:The
BRAND_NAVYconstant value is correct —rgb(1, 40, 81)maps to#012851matching--c-primaryinlayout.css. ✓Missing: visual regression tests. This PR changes the entire header appearance (background, nav state style, avatar color). There are no visual regression snapshots at the required breakpoints (320px, 768px, 1440px) in light and dark mode. These should exist before merge so future changes have a baseline.
Missing: unit test for
LanguageSwitcher invertedprop. The new boolean prop adds a conditional rendering branch inLanguageSwitcher.svelte. A Vitest component test would verify:text-white font-boldwheninverted=truetext-ink font-boldwheninverted=falsetext-white/55wheninverted=trueMissing: hamburger menu test at tablet breakpoints. The breakpoint change (sm → lg) means 768px–1023px now gets the mobile drawer. A test at
{ width: 768, height: 1024 }confirming the hamburger is visible (not the inline nav) would catch regressions.Login test correctly clears auth state with
storageState: { cookies: [], origins: [] }. ✓🔐 Nora "NullX" Steiner — Security Engineer
Verdict: ✅ Approved
This PR is a pure visual/CSS change — header background color, nav state styling, avatar colors, and a new auth page layout component. No authentication logic, no new endpoints, no data handling, no server-side changes.
What I Checked
forgot-password/+page.svelteandlogin/+page.svelteonly adopt a new header component, no changes to form actions or data flows.AuthHeader.sveltelinks tohref="/"only — no open redirect vectors.LanguageSwitcherinvertedprop — purely a CSS class toggle.setLocale()call is unchanged. No user input reaches any sink.brightness-0 invertCSS filter on the account icon — client-side SVG rendering, no security implications.Informational
The
<button type="button">on all locale switcher buttons is correct — withouttype="button", buttons inside a<form>default totype="submit"and could trigger unintended form submissions. On the auth pages this matters since there are login/forgot-password forms on the same page. ✓Nothing to block or flag here. Clean change.
🎨 Leonie Voss — UI/UX Design Lead
Verdict: 🚫 Changes requested
The navy header direction is right and I'm glad to see it. The mint avatar at 8.5:1 is excellent. Focus rings on all interactive elements — exactly what I asked for. But there's a WCAG AA failure I cannot approve.
Blockers
text-white/55fails WCAG AA for nav link text (CWE: WCAG 1.4.3 Contrast Minimum)Inactive nav items and inactive locale buttons use
text-white/55onbg-brand-navy(#012851). White at 55% opacity blended against#012851yields approximatelyrgb(141, 148, 168), giving a contrast ratio of ~3.0:1 against the navy background.text-xs(12px)font-bolduppercase — 12px bold does not qualify as "large text" under WCAG 2.2 (large text = ≥18px regular or ≥14px bold). So the 4.5:1 AA threshold applies.AuthHeader.Fix — raise opacity to meet 4.5:1:
The same fix must be applied consistently in:
AppNav.svelte— desktop nav links (inactive state)AppNav.svelte— mobile hamburger icon (text-white/70)LanguageSwitcher.svelte— inactive locale buttons (invertedbranch)AuthHeader.svelte— inactive locale buttonsNotificationBell.svelte—text-white/65is borderline; bell icon gets 3:1 UI component exception (WCAG 1.4.11) ✓ThemeToggle.svelte—text-white/65— same UI component exception applies ✓UserMenu.svelte— account iconopacity-65— UI component exception ✓text-white/70for text-level elements,text-white/65acceptable for icon-only UI components.Suggestions
Active
border-b-2underline positioning. Desktop nav links usemy-2 inline-flex items-center px-3. Themy-2(8px margin) means theborder-b-2 border-accentsits 8px above the header's bottom edge — it won't visually connect to the accent strip below the header. The effect is a floating underline in the middle of the bar rather than a tab-style indicator grounded at the bottom. Consider removingmy-2and usingpb-0.5orself-end pb-1to push the underline to the bottom of the h-16 container:Breakpoint shift sm → lg acknowledged. Tablets (768px–1023px) now always show the hamburger. This is an intentional design choice per the PR description. The implementation is consistent. I'm noting it as a UX trade-off: tablet users get a more mobile-like nav experience. Fine if that was deliberate.
AuthHeaderheight ish-12vs main headerh-16. The auth pages get a slimmer header — this reads well as a reduced-chrome context. ✓Logo always visible on mobile — good, this was missing before. ✓
Avatar
bg-brand-mint text-brand-navy— mint (#A6DAD8) on navy (#012851) text: this combination is used for the initials avatar, which hasbg-brand-mintbackground andtext-brand-navytext. #A6DAD8 bg with #012851 text: contrast ≈ 6.7:1. Excellent. ✓🚀 Tobias Wendt — DevOps & Platform Engineer
Verdict: ✅ Approved
No infrastructure, CI workflow, Docker Compose, or deployment configuration changes in this PR. Reviewing from a platform perspective: nothing to block.
What I Checked
package.jsonnot modified. Supply chain surface unchanged.frontend/e2e/— standard location matching existing Playwright setup. Tests use the same@playwright/testimport pattern as other e2e specs. ✓docker-compose.yml, CI workflow YAML, or Caddy/proxy config.Informational
The Playwright test hardcodes
BRAND_NAVY = 'rgb(1, 40, 81)'as a magic string. If the CSS custom property--c-primarychanges value, the test fails with a cryptic color-mismatch message. Not a blocker, but a comment or a named constant with the hex value alongside would help future maintainers understand what they're looking at:That's the only thing I'd note. Clean frontend-only PR from a platform standpoint.