feat(#90): add hamburger menu and mobile nav drawer below 640px #104

marcel · 2026-03-27T12:40:40+01:00

marcel commented

2026-03-27 12:40:40 +01:00

Closes #90

Summary

Nav links were hidden on mobile with no fallback (hidden sm:flex) — users below 640px had zero navigation
Added a 44×44px hamburger toggle button (mobile-only, sm:hidden) with inline SVG hamburger/X icons and full ARIA (aria-label, aria-expanded, aria-controls)
Added a fixed overlay panel (top-[68px], z-40) with full-width nav links (min-h-[44px] touch targets each), backdrop-click to close, Escape key to close, and $effect auto-close on route change

Test plan

On viewport < 640px: hamburger button is visible, nav links are hidden
Tapping hamburger opens the drawer with all nav links (Documents, Persons, Conversations, Admin if admin user)
Each nav link has ≥ 44px touch target height
Active route is visually indicated in the drawer
Tapping a link navigates and closes the drawer
Tapping the backdrop closes the drawer
Pressing Escape closes the drawer
On viewport ≥ 640px: hamburger is hidden, desktop nav links are visible, drawer never appears
Screen reader announces aria-expanded state on the toggle button

Closes #90 ## Summary - Nav links were hidden on mobile with no fallback (`hidden sm:flex`) — users below 640px had zero navigation - Added a 44×44px hamburger toggle button (mobile-only, `sm:hidden`) with inline SVG hamburger/X icons and full ARIA (`aria-label`, `aria-expanded`, `aria-controls`) - Added a fixed overlay panel (`top-[68px]`, `z-40`) with full-width nav links (min-h-[44px] touch targets each), backdrop-click to close, Escape key to close, and `$effect` auto-close on route change ## Test plan - [ ] On viewport < 640px: hamburger button is visible, nav links are hidden - [ ] Tapping hamburger opens the drawer with all nav links (Documents, Persons, Conversations, Admin if admin user) - [ ] Each nav link has ≥ 44px touch target height - [ ] Active route is visually indicated in the drawer - [ ] Tapping a link navigates and closes the drawer - [ ] Tapping the backdrop closes the drawer - [ ] Pressing Escape closes the drawer - [ ] On viewport ≥ 640px: hamburger is hidden, desktop nav links are visible, drawer never appears - [ ] Screen reader announces `aria-expanded` state on the toggle button

marcel added 1 commit 2026-03-27 12:40:40 +01:00

feat(#90 ): add hamburger menu and mobile nav drawer below 640px

CI / Unit & Component Tests (pull_request) Successful in 2m26s

Details

CI / Backend Unit Tests (pull_request) Successful in 2m11s

Details

CI / E2E Tests (pull_request) Failing after 25m59s

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

6ebae19984

Nav links were completely hidden on mobile (sm:flex / hidden split).
Adds a 44×44px hamburger toggle, a fixed overlay panel with full-width
nav links (min-h-[44px] touch targets), backdrop-click and Escape to
close, and a $effect that auto-closes on route change.

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

marcel merged commit 6ebae19984 into main

2026-03-27 15:48:18 +01:00

marcel deleted branch feature/90-mobile-hamburger-nav

2026-03-27 15:48:22 +01:00

marcel commented

2026-03-28 00:04:21 +01:00

Leonie Voss (@leonievoss) — UI/UX Review

I ran a full Playwright test suite against this PR at 320px viewport and also reviewed the code. Here's what I found.

What passes ✓

Hamburger button is visible at 320px, hidden at 1280px ✓
Touch target is 44×44px ✓
aria-expanded toggles correctly on click ✓
aria-label switches between "Menü öffnen" / "Menü schließen" ✓
Active route is visually highlighted in the drawer ✓
Desktop breakpoint hides the hamburger entirely ✓

The structure and ARIA wiring are solid. The intentions are right.

Critical bug — drawer resets immediately at `/` 🔴

4 of 11 E2E tests fail. The drawer does open — I can see it in the test screenshot for the aria-expanded test (X icon, all four nav links visible). But by the time any assertion on #mobile-nav runs, the element is gone from the DOM and the button is back to ≡. Something resets mobileNavOpen = false within milliseconds of it being set to true.

The culprit is the $effect in AppNav.svelte:

$effect(() => {
    void page.url.pathname;
    untrack(() => {
        mobileNavOpen = false;
    });
});

The intention is correct — close the nav after route changes. But page from $app/state is a deeply reactive proxy. At the home page (/), SvelteKit appears to trigger a reactive update to page (possibly from the document search load function completing its client-side revalidation, or from Paraglide updating locale metadata) that re-evaluates page.url.pathname after the user's click. This re-runs the $effect and resets mobileNavOpen = false.

This does NOT happen at /persons (the persons list is simpler) — that's why the active-route test at /persons passes while the equivalent test at / fails.

Fix: Replace the $effect with afterNavigate, which only fires on real navigation events, not on reactive re-renders of page:

import { afterNavigate } from '$app/navigation';
afterNavigate(() => {
    mobileNavOpen = false;
});

This is also cleaner semantics — it says exactly what it means: "close the nav after navigating."

Medium — `block flex` is redundant on every nav link 🟡

All four nav links in the mobile drawer use:

class="block flex min-h-[44px] w-full items-center ..."

flex sets display: flex. block sets display: block. They fight over the same CSS property; flex wins because it appears later in Tailwind's generated output. block is dead code here. Remove it from all four links.

Medium — Escape key handler won't fire from outside the overlay 🟡

<div class="fixed inset-0 top-[68px] z-40 sm:hidden" onkeydown={handleOverlayKeydown}>

onkeydown on a <div> only catches keyboard events from elements inside that div. When the drawer opens, focus remains on the hamburger button (which is outside the overlay). Pressing Escape fires on the button → bubbles to the <header> → stops there. It never reaches this div.

This means the Escape key does nothing. The test confirms it: the drawer never closes.

Fix option A — add a global keydown listener in an $effect:

$effect(() => {
    function onKeydown(e: KeyboardEvent) {
        if (e.key === 'Escape') mobileNavOpen = false;
    }
    document.addEventListener('keydown', onKeydown);
    return () => document.removeEventListener('keydown', onKeydown);
});

Fix option B — move focus into the panel on open (also better for screen readers):

<!-- Add tabindex="-1" and bind:this={panelEl}, then focus it after open -->

<!-- svelte-ignore a11y_click_events_have_key_events -->
<!-- svelte-ignore a11y_no_static_element_interactions -->
<div class="absolute inset-0 bg-black/20" onclick={closeMobileNav}></div>

Suppressing the warning doesn't fix the underlying issue: keyboard users and screen reader users who don't have a pointer cannot click a <div>. Once the Escape key handler is fixed (see above), this becomes less critical — keyboard users will have their dismissal path. But the ignore comments should be accompanied by a code comment explaining why it's acceptable (backdrop click is a progressive enhancement; keyboard path is via Escape).

z-index note — no bug, but worth knowing

The header has z-50, the overlay has z-40. The overlay is a DOM descendant of the header, so it lives inside the header's stacking context. position: fixed takes it out of layout flow, but NOT out of the stacking context. In practice this is fine — the overlay sits below the header visually (starts at top-[68px]) and above the page content. But if any future <main> content creates a stacking context with z-index > 40, the overlay could appear behind it. Consider moving the overlay render to a portal or raising it to z-50 to match the header.

Summary

Finding	Severity	File	Fix
Drawer resets immediately at `/` — 4 tests fail	Critical	`AppNav.svelte:11–18`	Replace `$effect` with `afterNavigate`
`block flex` on all four mobile nav links	Medium	`AppNav.svelte:145,155,165,177`	Remove `block`
Escape key handler unreachable	Medium	`AppNav.svelte:134`	Global `document` listener or focus management
Backdrop has suppressed a11y warnings without comment	Low	`AppNav.svelte:138`	Add explanatory comment

The hamburger bones are good. Touch targets, ARIA wiring, and desktop/mobile breakpoint split are all correct. Fix afterNavigate and this is shippable.

**Leonie Voss (@leonievoss) — UI/UX Review** I ran a full Playwright test suite against this PR at 320px viewport and also reviewed the code. Here's what I found. --- ## What passes ✓ - Hamburger button is visible at 320px, hidden at 1280px ✓ - Touch target is 44×44px ✓ - `aria-expanded` toggles correctly on click ✓ - `aria-label` switches between "Menü öffnen" / "Menü schließen" ✓ - Active route is visually highlighted in the drawer ✓ - Desktop breakpoint hides the hamburger entirely ✓ The structure and ARIA wiring are solid. The intentions are right. --- ## Critical bug — drawer resets immediately at `/` 🔴 **4 of 11 E2E tests fail.** The drawer _does_ open — I can see it in the test screenshot for the `aria-expanded` test (X icon, all four nav links visible). But by the time any assertion on `#mobile-nav` runs, the element is gone from the DOM and the button is back to ≡. Something resets `mobileNavOpen = false` within milliseconds of it being set to `true`. The culprit is the `$effect` in `AppNav.svelte`: ```svelte $effect(() => { void page.url.pathname; untrack(() => { mobileNavOpen = false; }); }); ``` The intention is correct — close the nav after route changes. But `page` from `$app/state` is a deeply reactive proxy. At the home page (`/`), SvelteKit appears to trigger a reactive update to `page` (possibly from the document search `load` function completing its client-side revalidation, or from Paraglide updating locale metadata) that re-evaluates `page.url.pathname` _after_ the user's click. This re-runs the `$effect` and resets `mobileNavOpen = false`. This does NOT happen at `/persons` (the persons list is simpler) — that's why the active-route test at `/persons` passes while the equivalent test at `/` fails. **Fix:** Replace the `$effect` with `afterNavigate`, which only fires on real navigation events, not on reactive re-renders of `page`: ```svelte import { afterNavigate } from '$app/navigation'; afterNavigate(() => { mobileNavOpen = false; }); ``` This is also cleaner semantics — it says exactly what it means: "close the nav after navigating." --- ## Medium — `block flex` is redundant on every nav link 🟡 All four nav links in the mobile drawer use: ```svelte class="block flex min-h-[44px] w-full items-center ..." ``` `flex` sets `display: flex`. `block` sets `display: block`. They fight over the same CSS property; `flex` wins because it appears later in Tailwind's generated output. `block` is dead code here. Remove it from all four links. --- ## Medium — Escape key handler won't fire from outside the overlay 🟡 ```svelte <div class="fixed inset-0 top-[68px] z-40 sm:hidden" onkeydown={handleOverlayKeydown}> ``` `onkeydown` on a `<div>` only catches keyboard events from elements _inside_ that div. When the drawer opens, focus remains on the hamburger button (which is _outside_ the overlay). Pressing Escape fires on the button → bubbles to the `<header>` → stops there. It never reaches this div. This means the Escape key does nothing. The test confirms it: the drawer never closes. **Fix option A** — add a global keydown listener in an `$effect`: ```svelte $effect(() => { function onKeydown(e: KeyboardEvent) { if (e.key === 'Escape') mobileNavOpen = false; } document.addEventListener('keydown', onKeydown); return () => document.removeEventListener('keydown', onKeydown); }); ``` **Fix option B** — move focus into the panel on open (also better for screen readers): ```svelte  ``` --- ## Low — suppressed a11y warnings on the backdrop 🟡 ```svelte   <div class="absolute inset-0 bg-black/20" onclick={closeMobileNav}></div> ``` Suppressing the warning doesn't fix the underlying issue: keyboard users and screen reader users who don't have a pointer cannot click a `<div>`. Once the Escape key handler is fixed (see above), this becomes less critical — keyboard users will have their dismissal path. But the ignore comments should be accompanied by a code comment explaining why it's acceptable (backdrop click is a progressive enhancement; keyboard path is via Escape). --- ## z-index note — no bug, but worth knowing The header has `z-50`, the overlay has `z-40`. The overlay is a DOM descendant of the header, so it lives inside the header's stacking context. `position: fixed` takes it out of layout flow, but NOT out of the stacking context. In practice this is fine — the overlay sits below the header visually (starts at `top-[68px]`) and above the page content. But if any future `<main>` content creates a stacking context with `z-index > 40`, the overlay could appear behind it. Consider moving the overlay render to a portal or raising it to `z-50` to match the header. --- ## Summary | Finding | Severity | File | Fix | |---|---|---|---| | Drawer resets immediately at `/` — 4 tests fail | Critical | `AppNav.svelte:11–18` | Replace `$effect` with `afterNavigate` | | `block flex` on all four mobile nav links | Medium | `AppNav.svelte:145,155,165,177` | Remove `block` | | Escape key handler unreachable | Medium | `AppNav.svelte:134` | Global `document` listener or focus management | | Backdrop has suppressed a11y warnings without comment | Low | `AppNav.svelte:138` | Add explanatory comment | The hamburger bones are good. Touch targets, ARIA wiring, and desktop/mobile breakpoint split are all correct. Fix `afterNavigate` and this is shippable.

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: marcel/familienarchiv#104

feat(#90): add hamburger menu and mobile nav drawer below 640px #104

Summary

Test plan

What passes ✓

Critical bug — drawer resets immediately at / 🔴

Medium — block flex is redundant on every nav link 🟡

Medium — Escape key handler won't fire from outside the overlay 🟡

Low — suppressed a11y warnings on the backdrop 🟡

z-index note — no bug, but worth knowing

Summary

Critical bug — drawer resets immediately at `/` 🔴

Medium — `block flex` is redundant on every nav link 🟡