feat(ui): implement responsive DocumentTopBar — final spec #173
Reference in New Issue
Block a user
Delete Branch "%!s()"
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?
Context
Implement the
DocumentTopBarcomponent per the final implementation spec atdocs/specs/document-topbar-final-spec.html. This spec supersedesdocument-topbar-b1-responsive.htmland incorporates all resolutions from the issue #161 team review.Open the spec in a browser before starting — it contains visual mockups, impl-ref tables, and acceptance criteria.
Component decomposition
Implement as six separate Svelte 5 components (never merge into a monolith):
DocumentTopBar.svelteoverflowOpen: $state(false). Passes props down.PersonChipRow.sveltehidden xs:flex. Receivessender,receivers,abbreviated.PersonChip.svelteabbreviated: booleanprop.OverflowPill.svelte<button>at ≥768px with tooltip;<span aria-hidden>at <768px.DocumentStatusChip.svelteAnnotateHintStrip.svelteannotateMode===trueAND ≥768px.Utility module
Create
src/lib/utils/personFormat.tswith these pure functions — write Vitest unit tests before implementing each:abbreviateName(person)— "Karl Raddatz" → "K. Raddatz"; single name → as-is; hyphenated last name preservedformatXsMeta(doc)— "K.Raddatz → E.Raddatz +2 · 24.12.1943"personAvatarColor(personId)— deterministic hash → one of['#012851','#5A3080','#007596','#2A6040','#803020']. UsePALETTE[hash(id) % 5]— never interpolate ID into CSS.formatDate(isoDate, 'short'|'long')— parse withT12:00:00suffix to avoid UTC off-by-onestatusDotClass(status)— Tailwind bg class perDocumentStatusstatusLabel(status)— German label string (fortitle+aria-labelonly)Responsive behaviour (authoritative — overrides B1 spec)
h-12(48px)h-14(56px)h-14(56px)text-[11px]text-[11px]text-[12px]/lg:text-[13px]text-[9px]text-[9px]<span aria-hidden>"+N"<button>"+N weitere" → tooltipannotateModeBack button touch target: visual icon 24–28px, wrapper
w-11 h-11 -ml-2(44×44px). Always<a>, never<button>.Date format:
<span class="lg:hidden">24.12.1943</span><span class="hidden lg:inline">{longDate}</span>— no JS viewport check.Design tokens
All colours via CSS custom properties — no hardcoded hex in components.
bg-surfacebg-surfaceborder-lineborder-linebg-primary/ accent barborder-l-[3px] border-primarybg-mutedbg-mutedtext-inktext-inktext-ink-2text-ink-2⚠
--color-primarymust be RGB channels (1 40 81), not hex, for Tailwind opacity modifiers (/5,/20) to work in the hint strip. If conversion is risky, use explicitbg-[rgba(1,40,81,0.05)]instead.Annotate mode
When
annotateMode === true:aria-pressed=true{#if annotateMode}, no CSS animation)Hint strip:
hidden md:flex h-[18px] items-center gap-2 border-t border-dashed px-3.5 bg-[rgba(1,40,81,0.05)] border-[rgba(1,40,81,0.20)]Overflow tooltip (≥768px only)
absolute top-full left-0 mt-1 bg-surface border border-line rounded-md shadow-lg p-3 min-w-[160px] z-50role="listbox"on panel;role="option"on each person row (link to/persons/{id})text-[9px] font-bold uppercase tracking-wide text-ink-2role="option"; Tab/Shift+Tab navigates; Escape closes + returns focus to pilluse:clickOutsideSvelte action — notdocument.addEventListenerin$effectaria-haspopup="listbox"(not"true"),aria-expanded={overflowOpen}Tailwind config
Add custom breakpoint to
tailwind.config.ts:⚠ Tailwind 4 may use different syntax — verify before using
xs:prefix classes.Accessibility
aria-label="Zurück zur Dokumentenliste"alwaysaria-label="Bearbeiten"always (even icon-only)aria-pressed={annotateMode}aria-haspopup="listbox",aria-expanded,aria-label="{count} weitere Empfänger anzeigen"title={statusLabel}+aria-label={statusLabel}→:aria-hidden="true"alwaysfocus-visible:ring-2 focus-visible:ring-primary— neveroutline:nonewithout replacementOut of scope
AnnotationSidePanel)AnnotationSidePanelorDocumentBottomPaneldoc.locationfield if not yet on backend DTO — leave// TODO: show location when availableAcceptance criteria
/persons/{id}<span>— no tooltiparia-pressed,aria-haspopup="listbox",aria-expanded,aria-labelon all interactive elementssvelte-checkpasses with no new type errors/proofshotat all 5 viewport widths × 2 themes = 10 screenshotsReference
Final spec (authoritative):
docs/specs/document-topbar-final-spec.html— open in browserVisual reference:
docs/specs/document-topbar-b1-responsive.htmlExisting component:
src/lib/components/DocumentTopBar.svelteConsuming page:
src/routes/documents/[id]/+page.svelteReview thread: #161
👨💻 Felix Brandt — Senior Fullstack Developer
Questions & Observations
TDD scope is partially specified. The issue explicitly calls for Vitest unit tests before implementing each of the 6 utility functions — good. But what about component-level tests?
PersonChip,PersonChipRow,OverflowPillall have conditional rendering logic (abbreviated names, span-vs-button, aria attributes) that deserves unit test coverage beyond the utility layer. Is the expectation that the Vitest utility tests + the 10 proofshots cover this, or should there be component tests as well?OverflowPillis doing two visually distinct things. At ≥768px it's a<button>with a tooltip; below that it's a<span aria-hidden>. The spec says these live in one component. That meansOverflowPillcontains layout-conditional rendering logic and interaction logic — two responsibilities. Consider whetherOverflowPillButtonandOverflowPillDisplayshould be separate, withPersonChipRowconditionally rendering the right one. I'd ask Leonie whether the spec intent was "one file for co-location" or "one component doing both jobs."The
clickOutsideSvelte action — is this action already in the project, or does it need to be created? If it needs to be written, its implementation needs its own unit test first (red/green applies to actions too). Usingdocument.addEventListenerinside a Svelte action is fine — that's exactly the pattern actions are designed for — but the teardown in thedestroycallback must be tested.personAvatarColorhash function — "deterministic hash → one of 5 palette entries" is under-specified. What hash algorithm?charCodeAtsum mod 5 will produce a skewed distribution for UUIDs (which start with the same hex groups). A simple djb2 or FNV-1a over the full ID string would distribute more evenly. The test suite should cover: same ID always returns same color, all 5 colors are reachable.abbreviated: booleanonPersonChip— this is a prop, not a method argument, so the boolean-flag rule from the style guide doesn't apply. No concern there.Keyed
{#each}blocks — the receivers list inPersonChipRowmust key byreceiver.id. If any implementation iterates receivers without a key expression, that's a rule violation.Suggestions
clickOutsideaction test first, then implement it, before startingOverflowPill. It unblocks the tooltip work and keeps TDD clean.formatXsMeta, write tests for: no sender + 0 receivers, sender + 0 receivers, sender + 1 receiver, sender + 5 receivers — the edge cases that affect the arrow character and the+Ncount.$derivedrune should be used for any computed value inDocumentTopBar(e.g. overflow count, abbreviated names). Flag any$state+$effectpattern that computes a value — that's a rule violation regardless of how small the component is.🏛️ Markus Keller — Application Architect
Questions & Observations
Where do the design tokens live? The spec references
bg-surface,border-line,text-ink,text-ink-2,bg-mutedas Tailwind classes backed by CSS custom properties. Are these already defined inlayout.css(the existing brand token file), or does this issue introduce them? If they're new tokens, that's a non-trivial CSS change that touches the entire app's theme layer — the scope should be explicit, and the tokens should be added in a separate, reviewable commit.--color-primaryas RGB channels is a design token contract with the Tailwind config. The spec correctly flags this (1 40 81not#002850), but it needs to be established before any component tries to use/5or/20opacity modifiers. Is the existingbrand-navytoken already in RGB channel format, or is this a migration?The
xs: '375px'breakpoint intailwind.config.ts— Tailwind 4 moved away fromtailwind.config.tsin favour of@themedirectives in CSS. The spec flags this uncertainty ("⚠ Tailwind 4 may use different syntax — verify before usingxs:prefix classes"). Before implementing anything that depends onxs:flexorhidden xs:flex, this config question must be resolved. A component that conditionally hides onxs:will silently fail if the breakpoint is never registered.role="listbox"/role="option"on the overflow tooltip — this ARIA pattern implies a selection widget, where one option is "selected" and the widget communicates a chosen value. The overflow tooltip is really a navigation list (links to person pages), not a listbox.role="menu"/role="menuitem"(or simplyrole="list"/role="listitem"with links) would be semantically more accurate. This is worth checking against the ARIA spec before implementation, as it affects the expected keyboard interaction model.use:clickOutsideaction — if this doesn't already exist insrc/lib/actions/, it needs to be created as part of this issue. Its placement should besrc/lib/actions/clickOutside.tsto stay consistent with how other shared utilities are organized. It should not live inside the component file itself.Module boundary is clean. The utility module
src/lib/utils/personFormat.tsis a pure-function module with no component dependencies — correct layering. The six components pass data via props only. No concerns about domain coupling here.Suggestions
xsbreakpoint question before writing a single line of component code. A quicknpm run buildwith anxs:flexclass present will confirm whether it's registered and purged correctly.bg-surface,text-inketc. are new tokens: introduce them in a dedicated "add dark-mode design tokens" commit that is separate from the component work. That makes the token surface area reviewable independently.role="listbox"intent with accessibility sources — if it should berole="menu", the keyboard interaction model (arrow keys vs Tab) changes, and that affects focus management implementation.🧪 Sara Holt — QA Engineer
Questions & Observations
Unit tests (specified):
The issue specifies unit tests for all 6 utility functions. The acceptance criteria mention "all edge cases per spec Section 6." I want to make sure the following cases are explicitly covered before the PR lands:
abbreviateName"Karl-Heinz Raddatz"), empty string, name with trailing/leading spacesformatXsMeta+N), 2+ receivers (+N count correct), no datepersonAvatarColorformatDateT12:00:00suffix prevents off-by-one at UTC boundary — test at Dec 31, Jan 1 boundary datesstatusDotClassDocumentStatusenum values covered — no unhandled casestatusLabelDocumentStatusenum values covered, returns German stringThe acceptance criteria list receiver cases as "0, 1, 2, 3, 5" — 4 receivers is not listed. Was that intentional? If 4 is not a special case, it should still be tested to verify the
+Narithmetic is correct.Component-level tests (not specified):
The issue only mentions utility unit tests. These component behaviors are not covered by utility tests alone:
OverflowPillrenders as<span aria-hidden>below 768px and<button>above — this conditional rendering should have a Vitest component test.DocumentStatusChiphidden below 768px — testing this with CSS visibility requires either an E2E test or a component test that checks rendered class names.AnnotateHintStriponly rendered whenannotateMode === true— a{#if}block: straightforward component test.Acceptance criteria gap:
getBoundingClientRect()for the back button and edit button would make this deterministic.svelte-checkpasses is listed as an acceptance criterion — good, this is a CI gate. But it should already be passing before the PR opens, not just "passes at some point."Suggestions
src/lib/utils/personFormat.test.tsas the first commit, with all test skeletons in place (failing) before any implementation. This makes the TDD intent visible in the commit graph.@testing-library/sveltecomponent tests forOverflowPillandAnnotateHintStrip— they have branching render logic that's hard to cover with screenshots alone.role=\"option\"") — this is a Playwright E2E test, not a unit test. Should it be added to the existing document detail E2E suite?🔒 Nora "NullX" Steiner — Security Engineer
Questions & Observations
Overall, this is a pure frontend rendering component with no new backend surface — the risk profile is low. That said, a few things are worth flagging:
personAvatarColor— CSS injection guard (correctly specified, verify in implementation)The spec explicitly states: "never interpolate ID into CSS" and mandates
PALETTE[hash(id) % 5]— returning a hex string from a static array, not inserting the ID into a CSS string. This is the right pattern. In Svelte,style="background: {color}"binds a value from a trusted palette array, not user input — no XSS vector here as long as the implementation follows the spec. Worth a quick implementation review to confirm no one sneaks instyle="background: {person.id}"by accident.Overflow tooltip: person name rendering
The tooltip renders
person.nameas link text inside the panel. Svelte's template compiler auto-escapes text bindings ({person.name}), so stored XSS is not a concern here — as long as no{@html}is used anywhere in the overflow panel. Flag during PR review: search for{@html}in all 6 new component files.clickOutsideaction implementationThe spec mandates
use:clickOutsiderather thandocument.addEventListenerdirectly in$effect. This is the safer pattern — the action teardown is guaranteed by Svelte's lifecycle, preventing event listener leaks. The main risk in a customclickOutsideimplementation is checkingcontains()incorrectly, which can cause the tooltip to close when clicking inside it (UX bug, not a security bug). Not a vulnerability, but worth a careful implementation./persons/{id}links in the tooltipThese links use person UUIDs from the backend DTO. UUIDs are safe in URL paths — no injection vector. Svelte's
hrefbinding handles this safely.Focus management and keyboard trap risk
The spec describes focus moving into the tooltip on open, with Tab/Shift+Tab navigating within it, and Escape returning focus to the pill. This is correct — it's not a focus trap (Tab can exit). However: if the implementation accidentally creates a true focus trap (Tab loops indefinitely inside the tooltip), it would be an accessibility failure, not a security issue. Verify the Tab-out behavior in testing.
No new backend endpoints, no auth changes, no data exposure — this issue does not expand the app's attack surface.
Suggestions
{@html}in all 6 new.sveltefiles — confirm zero occurrences.personAvatarColorreturns from a static palette array, not a constructed CSS string.clickOutsideaction deserves a code review eye specifically on theevent.targetcheck — confirm it usesnode.contains(event.target)and handles shadow DOM or portal cases if any exist in the app.🎨 Leonie Voss — UI/UX Designer
Questions & Observations
Font sizes below 12px minimum — needs resolution before implementation
The spec specifies:
text-[9px]for chip name text at all breakpoints ≥375pxtext-[11px]for title font at XS and MobileBoth values are below my 12px floor. At
text-[9px]chip names will be illegible for older users on any display, and at non-retina resolution this will render as blurry subpixel text. I understand the space constraint is real — the topbar is only 48–56px tall — but I need to know: was this a deliberate trade-off in the final spec, or is it a scaled-down mockup value that accidentally ended up in the implementation reference table?If it was intentional: document the rationale (e.g. "chip names are decorative/supplemental, the primary content is the title"). If it was accidental: the minimum acceptable chip name size is
text-[10px]/text-xs(10–12px), and the title should be at leasttext-xs(12px) at XS.hidden xs:flexand the XS chip rowAt <375px (XS), the chip row is hidden and replaced with
xsMetaLine(theformatXsMetaoutput). What is the visual treatment ofxsMetaLine? The spec defines the utility function but I don't see where the resulting string is rendered or styled in the component breakdown. Is it a secondary line below the title? What's its font size and color? This rendering path needs to be in the spec or implementation notes.Status dot — "dot only, no text label"
At ≥768px the status indicator shows a colored dot with no visible label — only
titleandaria-label. For users who are unfamiliar with the color coding, this is an undiscoverable UI element. Color alone is not sufficient per WCAG 1.4.1. The dot-only approach is acceptable if and only if it's explicitly supplemental (users can discover the status elsewhere, e.g. on the document detail page). Can you confirm this constraint is met by the surrounding page layout?Annotate hint strip at 18px height
h-[18px]is extremely compact. At 56px topbar + 18px strip = 74px total header height on tablet — this is tight but workable. My concern is the strip content:gap-2with icon and text at what font size? The spec shows the class string but not the text size inside the strip. Anything belowtext-[10px]in an 18px strip will be unreadable.Touch targets
Back button at
w-11 h-11(44×44px) — correct. Edit button at icon-only on mobile — what is the actual touch target size? The spec mentions icon-only rendering but doesn't specify wrapper dimensions. The visual icon may be 24px but the tap target must be 44×44px minimum.focus-visible:ring-2 focus-visible:ring-primaryon all interactive elementsThis is the right pattern. Confirm during implementation that
ring-primaryresolves correctly — if--color-primaryis in RGB channel format (1 40 81), thenring-primaryneeds to be declared as a Tailwind color utility in the config, not just a CSS variable. Verify this before review.Suggestions
text-[9px]for chips a final product decision, or a leftover scaled value?xsMetaLinerendering location, font size, and color class.min-w-[44px] min-h-[44px](or equivalent) to the wrapper, not just the visible icon size.🛠️ Tobias Wendt — DevOps & Platform Engineer
Questions & Observations
This is a pure frontend component issue — no backend changes, no infrastructure changes. My concerns are limited but concrete:
Tailwind 4 custom breakpoint syntax
The spec correctly flags this as a risk: "⚠ Tailwind 4 may use different syntax — verify before using
xs:prefix classes." In Tailwind 4, thetailwind.config.tstheme.extend.screensapproach is replaced by@themeCSS directives:If
tailwind.config.tsdoesn't exist in this project (Tailwind 4 dropped it), adding anxsbreakpoint there will have no effect. Thexs:flexandhidden xs:flexclasses will be treated as unknown and silently stripped by the optimizer. The result: the chip row visibility logic breaks at runtime with no build error. This must be verified against the actual Tailwind version inpackage.jsonbefore anyxs:class is used.npm run build+svelte-checkin CIBoth are already in the pipeline. The
svelte-checkAC criterion is covered. No new CI jobs are needed for this issue.10 proofshots as acceptance artifacts
The
/proofshotprocess produces screenshots at 5 viewports × 2 themes. These live inproofshot-artifacts/. They're not automated visual regression — they're manual sign-off artifacts. That's fine for a small team, but worth noting: if the topbar is modified again in a future issue, there's no baseline diff to catch regressions. Something to consider when the visual test story matures.src/lib/utils/personFormat.tsas a new moduleNo infrastructure or build config changes required — Vite and SvelteKit pick this up automatically. The new
src/lib/actions/clickOutside.ts(if created) is the same story.No new environment variables, no new dependencies (likely)
Unless
clickOutsideis pulled from a package likesvelte-legosor@svelte-put/clickoutside, nopackage.jsonchanges are expected. If a dependency is added, it should be pinned to a specific version, not a range, given this is a production app.Suggestions
npx tailwindcss --versionand checkpackage.jsonto confirm Tailwind 4. If Tailwind 4: use@theme { --breakpoint-xs: 375px; }in the CSS entry point, nottailwind.config.ts.clickOutsideis brought in as a package, pin the exact version and runnpm auditafter install.👨💻 Felix Brandt — Senior Fullstack Developer
Follow-up discussion resolving open implementation questions raised by the team review.
Resolved
clickOutsideaction — extract to shared modulesrc/lib/actions/does not exist yet; the action is currently copy-pasted inline into 5 components (PersonTypeahead,PersonMultiSelect,TagInput,UserMenu,CorrespondentSuggestionsDropdown)src/lib/actions/clickOutside.ts, write Vitest test first (red/green), then replace all 5 inline copies with the shared importpersonAvatarColorhash algorithm — djb2Math.abs(hash) % 5Tailwind 4
xsbreakpoint — confirmed approachpackage.json;tailwind.config.tsapproach has no effect--breakpoint-xsis not yet defined anywhere in the codebase;xs:classes are not used anywhere--breakpoint-xs: 375px;to the existing@themeblock insrc/routes/layout.css— prerequisite before anyxs:flexclass is writtenOverflowPillsplit — two componentsOverflowPillButton(≥768px, interactive<button>with tooltip) andOverflowPillDisplay(<768px, non-interactive<span aria-hidden>);PersonChipRowconditionally renders the right onerole="listbox"→role="list"/role="listitem"/persons/{id}), not a selection widget —role="listbox"is semantically incorrectrole="list"on the panel,role="listitem"on each row, standard<a>links;aria-haspopup="true"on the pill buttonxsMetaLinerendering — not rendered in topbarformatXsMetaoutput is undocumented in the specformatXsMetais implemented and tested as a pure utility function but has no call site in this PRDesign tokens — already defined, no new work needed
bg-surface,bg-muted,border-line,text-ink,text-ink-2,bg-primary,border-primaryare all already defined insrc/routes/layout.cssvia@theme inline--color-primarystays as hex; hint strip uses explicitbg-[rgba(1,40,81,0.05)]andborder-[rgba(1,40,81,0.20)]per spec fallback — no token format migration neededComponent tests — in scope
@testing-library/sveltecomponent tests forOverflowPillButton(button/tooltip conditional rendering) andAnnotateHintStrip({#if annotateMode}branch); TDD applies — failing component test before implementationOverall: the spec is implementable as written with these 8 clarifications. The main structural deviations from the spec are the
OverflowPillsplit (item 4) and the ARIA role correction (item 5). Everything else is additive or confirmatory.