Animate and dim the add-place panel, fit its fields to the sidebar (tasks 180/181) #117

Merged

rob merged 3 commits from feat/add-place-panel-polish into main 2026-08-11 10:34:03 +00:00

Conversation 2 Commits 3 Files changed 13 +949 -37

rob commented 2026-08-11 09:49:39 +00:00

Owner

Copy link

Two related tickets, one pass over PlaceFormPanel/PlaceForm since both touch the same files.

Task 180. The backdrop and fade/slide are the visual expression of a distinction the code already draws (ADR-0099, merged hours earlier): PlaceFormPanel is a modal task and keeps its OverlayFocusTrap, unlike PlaceDetailPanel. ADR-0099's own Context section already predicted this ticket by number — checked against what's actually built here, and the wording still holds, so no amendment needed.

• New .place-form-panel-in/-out keyframes (opacity + transform only), removed outright under prefers-reduced-motion rather than shortened.
• New backdrop reuses the existing --colour-backdrop token (Drawer's own scrim) rather than adding a second one for the same kind of dimming.
• Click-away, the close button and Escape all route through one RequestCloseAsync: a clean form closes immediately; a dirty one (PlaceForm.IsDirty, new) shows an inline "Discard your unsaved changes?" confirmation instead, matching PlaceDetailPanel's own confirmation shape. Escape cancels a showing confirmation first, mirroring the precedence task 177 established there — the two routes now agree, as the ticket asks.
• Closing waits ~200ms (declared once in C#, once in CSS — no way to read a CSS animation's duration back from Blazor) so the exit animation has time to play before the host removes the panel; OverlayFocusTrap stays engaged for that whole window.
• A successful save closes directly, bypassing the dirty check — nothing to discard.

Task 181. Investigated before writing CSS: every field was already correctly flex-stretched to .auth-form's own width; the sole cause of "doesn't fit the sidebar" was forms.css's shared max-width: 24rem, sized for Login/Register/Account, not this wider panel. Overridden for .place-form only — forms.css itself, and every other form using it, is untouched. Lat/long: deleted the flex: 1 1 0 that forced equal-stretch (a plain flex item already shrinks-to-content by default), gave the inputs width: 12ch, added flex-wrap as a defensive measure against the narrowest supported width. Same component serves add and edit, so both paths get the fix from the same rule — checked directly against both.

Appearance itself is unverified — bUnit asserts classes and markup, not layout or animation (ADR-0090, the accessibility audit). Task 156 covers the visual pass.

Two related tickets, one pass over `PlaceFormPanel`/`PlaceForm` since both touch the same files. **Task 180.** The backdrop and fade/slide are the visual expression of a distinction the code already draws (ADR-0099, merged hours earlier): `PlaceFormPanel` is a modal task and keeps its `OverlayFocusTrap`, unlike `PlaceDetailPanel`. ADR-0099's own Context section already predicted this ticket by number — checked against what's actually built here, and the wording still holds, so no amendment needed. - New `.place-form-panel-in`/`-out` keyframes (opacity + transform only), removed outright under `prefers-reduced-motion` rather than shortened. - New backdrop reuses the existing `--colour-backdrop` token (Drawer's own scrim) rather than adding a second one for the same kind of dimming. - Click-away, the close button and Escape all route through one `RequestCloseAsync`: a clean form closes immediately; a dirty one (`PlaceForm.IsDirty`, new) shows an inline "Discard your unsaved changes?" confirmation instead, matching `PlaceDetailPanel`'s own confirmation shape. Escape cancels a showing confirmation first, mirroring the precedence task 177 established there — the two routes now agree, as the ticket asks. - Closing waits ~200ms (declared once in C#, once in CSS — no way to read a CSS animation's duration back from Blazor) so the exit animation has time to play before the host removes the panel; `OverlayFocusTrap` stays engaged for that whole window. - A successful save closes directly, bypassing the dirty check — nothing to discard. **Task 181.** Investigated before writing CSS: every field was already correctly flex-stretched to `.auth-form`'s own width; the sole cause of "doesn't fit the sidebar" was `forms.css`'s shared `max-width: 24rem`, sized for Login/Register/Account, not this wider panel. Overridden for `.place-form` only — `forms.css` itself, and every other form using it, is untouched. Lat/long: deleted the `flex: 1 1 0` that forced equal-stretch (a plain flex item already shrinks-to-content by default), gave the inputs `width: 12ch`, added `flex-wrap` as a defensive measure against the narrowest supported width. Same component serves add and edit, so both paths get the fix from the same rule — checked directly against both. Appearance itself is unverified — bUnit asserts classes and markup, not layout or animation (ADR-0090, the accessibility audit). Task 156 covers the visual pass.

rob added 1 commit 2026-08-11 09:49:39 +00:00

Animate and dim the add-place panel, and fit its fields to the sidebar 87e254839b

rob reviewed 2026-08-11 10:00:58 +00:00

rob left a comment

Copy link

Verdict: changes needed

CI is red on this head commit (run #520, failure). I don't have a tool that exposes raw job logs, and the Actions UI didn't render through fetch, so I can't quote the failing step directly. What I could do: reproduce the build job's own steps locally with the pinned SDK — full solution build, dotnet test PlaceMark.slnx (556 WebUI + 102 Architecture, all green), the coverage ratchet (WebUI 90% against the 87.9% baseline), and dotnet format --verify-no-changes — all clean, and none of build's other steps (DB provisioning, schema) touch anything this PR changes. That narrows suspicion to the e2e job, which I could not reproduce: its own fixture deliberately binds fixed ports (5017/5017/5169, not configurable — PlaceMarkAppFixture's own doc comment) and both were already held by long-running processes on this machine unrelated to any worktree, so every attempt failed on AddressInUseException before the app under test even started — not a finding about this PR's code, just an environment collision I can't resolve without touching processes I don't own. Given the PR's own changes are exactly the kind of thing an E2E journey clicking through the add-place panel would be sensitive to (a new closing delay, a new backdrop that could intercept a click), I can't rule out a genuine failure here, and a red CI run has to block on its own regardless. Please check the actual run before merging, or say what it was if you already know.

Priority 1 — the two routes are wired identically in the code, but the test suite doesn't independently prove it, and the gap is real. RequestCloseAsync is genuinely the one method the backdrop, the close button and (via HandlePanelKeyDownAsync, once no confirmation is showing) Escape all call — confirmed by reading every call site, not just the doc comments. Mutation-tested the dirty check itself: inverting IsDirty == true to == false reddens 6 tests including the named PlaceFormPanel_BackdropClickedOnADirtyForm_ShowsTheDiscardConfirmationRatherThanClosing, and the clean-form converse (_BackdropClickedOnACleanForm_, _EscapePressedOnACleanForm_) both pass at baseline and both redden under that mutation, so a clean form dismissing without friction is genuinely proven on both routes.

But PlaceFormPanel_BackdropClickedOnADirtyForm_...'s own doc comment claims Escape's dirty-form behaviour is "proved separately below" — it isn't. There is no test anywhere in this file that presses Escape on a dirty, non-confirming form and asserts the confirmation appears (only _EscapePressedWhileConfirmingDiscard_, which is Escape cancelling an already-open confirmation, a different case). I mutated HandlePanelKeyDownAsync so Escape calls StartClosingAsync() directly instead of RequestCloseAsync() — i.e., Escape unconditionally discards a dirty form, bypassing the confirmation entirely — and all 17 PlaceFormPanelTests still passed. That's exactly the failure mode flagged as worse than no gate at all: today it doesn't fire only because nobody has made that one-line change, not because a test would catch it if someone did. Add a test pressing Escape on a dirty form and asserting the confirmation shows; the doc comment's own claim says it should already exist.

Priority 2 — this is a small, cheap fix, and I'd ask for it. layout.js's matches(query) export is not specific to the viewport breakpoint — it's window.matchMedia(query).matches for whatever query string is passed, already used generically by Drawer.razor.cs for its own default-open check. StartClosingAsync could call the same function with "(prefers-reduced-motion: reduce)" and skip Task.Delay entirely when it matches, at the cost of one more await, reusing infrastructure this codebase already imports and already pays the "one interop round trip" cost for elsewhere without complaint. The ADR's framing conflates this with the rejected animationend/transitionend listener alternative — that one is a genuinely new, event-driven interop surface; reading a static media query is a single boolean read this project has already built and already uses for the same kind of decision. It's also not just a "feels slow" complaint: under reduced motion the panel visually snaps away instantly (CSS-wise) but stays mounted — and keeps OverlayFocusTrap engaged — for a further 200ms doing nothing, a real mismatch between what's shown and what's still trapping focus, not only a perception issue.

Priority 3.

• ADR-0027: confirmed directly, not just via NoBorderTokenOverrideTests (which only guards the two border tokens, as flagged — read it myself to confirm the scope). Grepped every touched CSS/razor file for hex/rgba() literals: none. --colour-backdrop is reused unchanged from its existing declaration; only its comment was widened. Clean.
• Both paths: RequestCloseAsync/StartClosingAsync/HandlePanelKeyDownAsync never branch on Place/IsEdit, so the dismiss behaviour applies to editing structurally — but the dismiss tests in PlaceFormPanelTests are all Add-mode only (none pass Place), so this rests on the same kind of code-sharing argument as the Escape gap above, just with less risk of an easy accidental divergence. The layout fix (.place-form-coordinates, .place-form-map) is unconditional in PlaceForm.razor's markup too — verified by reading the file — though again there's no dedicated new test asserting those class hooks in both IsEdit states; IsDirty itself is thoroughly tested both ways, including the "changed then restored" edge case.
• ADR-0099's prediction: read both records side by side. ADR-0099's own Context text — "task 180 will shortly give the add-place panel... a visibly stronger modal treatment... animating it in and out, dimming the background behind it, and dismissing it on a click away from the panel" — matches what's actually built here in every particular (fade/slide animation, backdrop dimming, click-away dismissal). The claim holds; no amendment was needed.
• Confirmed nothing in the tests or ADR implies appearance was checked — the Consequences section states the same limit plainly (task 156, ADR-0090's own precedent), and nothing in PlaceFormPanelTests/PlaceFormTests asserts computed layout or animation, only classes and markup.

Verdict: changes needed **CI is red on this head commit (run #520, `failure`).** I don't have a tool that exposes raw job logs, and the Actions UI didn't render through fetch, so I can't quote the failing step directly. What I could do: reproduce the `build` job's own steps locally with the pinned SDK — full solution build, `dotnet test PlaceMark.slnx` (556 WebUI + 102 Architecture, all green), the coverage ratchet (WebUI 90% against the 87.9% baseline), and `dotnet format --verify-no-changes` — all clean, and none of `build`'s other steps (DB provisioning, schema) touch anything this PR changes. That narrows suspicion to the `e2e` job, which I could not reproduce: its own fixture deliberately binds fixed ports (5017/5017/5169, not configurable — `PlaceMarkAppFixture`'s own doc comment) and both were already held by long-running processes on this machine unrelated to any worktree, so every attempt failed on `AddressInUseException` before the app under test even started — not a finding about this PR's code, just an environment collision I can't resolve without touching processes I don't own. Given the PR's own changes are exactly the kind of thing an E2E journey clicking through the add-place panel would be sensitive to (a new closing delay, a new backdrop that could intercept a click), I can't rule out a genuine failure here, and a red CI run has to block on its own regardless. Please check the actual run before merging, or say what it was if you already know. **Priority 1 — the two routes are wired identically in the code, but the test suite doesn't independently prove it, and the gap is real.** `RequestCloseAsync` is genuinely the one method the backdrop, the close button and (via `HandlePanelKeyDownAsync`, once no confirmation is showing) Escape all call — confirmed by reading every call site, not just the doc comments. Mutation-tested the dirty check itself: inverting `IsDirty == true` to `== false` reddens 6 tests including the named `PlaceFormPanel_BackdropClickedOnADirtyForm_ShowsTheDiscardConfirmationRatherThanClosing`, and the clean-form converse (`_BackdropClickedOnACleanForm_`, `_EscapePressedOnACleanForm_`) both pass at baseline and both redden under that mutation, so a clean form dismissing without friction is genuinely proven on both routes. But `PlaceFormPanel_BackdropClickedOnADirtyForm_...`'s own doc comment claims Escape's dirty-form behaviour is "proved separately below" — it isn't. There is no test anywhere in this file that presses Escape on a dirty, non-confirming form and asserts the confirmation appears (only `_EscapePressedWhileConfirmingDiscard_`, which is Escape *cancelling an already-open* confirmation, a different case). I mutated `HandlePanelKeyDownAsync` so Escape calls `StartClosingAsync()` directly instead of `RequestCloseAsync()` — i.e., Escape unconditionally discards a dirty form, bypassing the confirmation entirely — and all 17 `PlaceFormPanelTests` still passed. That's exactly the failure mode flagged as worse than no gate at all: today it doesn't fire only because nobody has made that one-line change, not because a test would catch it if someone did. Add a test pressing Escape on a dirty form and asserting the confirmation shows; the doc comment's own claim says it should already exist. **Priority 2 — this is a small, cheap fix, and I'd ask for it.** `layout.js`'s `matches(query)` export is not specific to the viewport breakpoint — it's `window.matchMedia(query).matches` for whatever query string is passed, already used generically by `Drawer.razor.cs` for its own default-open check. `StartClosingAsync` could call the same function with `"(prefers-reduced-motion: reduce)"` and skip `Task.Delay` entirely when it matches, at the cost of one more `await`, reusing infrastructure this codebase already imports and already pays the "one interop round trip" cost for elsewhere without complaint. The ADR's framing conflates this with the rejected `animationend`/`transitionend` listener alternative — that one is a genuinely new, event-driven interop surface; reading a static media query is a single boolean read this project has already built and already uses for the same kind of decision. It's also not just a "feels slow" complaint: under reduced motion the panel visually snaps away instantly (CSS-wise) but stays mounted — and keeps `OverlayFocusTrap` engaged — for a further 200ms doing nothing, a real mismatch between what's shown and what's still trapping focus, not only a perception issue. **Priority 3.** - ADR-0027: confirmed directly, not just via `NoBorderTokenOverrideTests` (which only guards the two border tokens, as flagged — read it myself to confirm the scope). Grepped every touched CSS/razor file for hex/`rgba()` literals: none. `--colour-backdrop` is reused unchanged from its existing declaration; only its comment was widened. Clean. - Both paths: `RequestCloseAsync`/`StartClosingAsync`/`HandlePanelKeyDownAsync` never branch on `Place`/`IsEdit`, so the dismiss behaviour applies to editing structurally — but the dismiss tests in `PlaceFormPanelTests` are all Add-mode only (none pass `Place`), so this rests on the same kind of code-sharing argument as the Escape gap above, just with less risk of an easy accidental divergence. The layout fix (`.place-form-coordinates`, `.place-form-map`) is unconditional in `PlaceForm.razor`'s markup too — verified by reading the file — though again there's no dedicated new test asserting those class hooks in both `IsEdit` states; `IsDirty` itself is thoroughly tested both ways, including the "changed then restored" edge case. - ADR-0099's prediction: read both records side by side. ADR-0099's own Context text — "task 180 will shortly give the add-place panel... a visibly stronger modal treatment... animating it in and out, dimming the background behind it, and dismissing it on a click away from the panel" — matches what's actually built here in every particular (fade/slide animation, backdrop dimming, click-away dismissal). The claim holds; no amendment was needed. - Confirmed nothing in the tests or ADR implies appearance was checked — the Consequences section states the same limit plainly (task 156, ADR-0090's own precedent), and nothing in `PlaceFormPanelTests`/`PlaceFormTests` asserts computed layout or animation, only classes and markup.

rob added 2 commits 2026-08-11 10:27:39 +00:00

Merge remote-tracking branch 'origin/main' into feat/add-place-panel-polish 924a2ecd2e

Stop the closing panel colliding with what opens next, and skip its delay under reduced motion 9f60df6683

rob reviewed 2026-08-11 10:30:09 +00:00

rob left a comment

Copy link

Verdict: mergeable

Reviewed by the orchestrating agent rather than a separate reviewing subagent — the session's subagent limit is exhausted. Independent of the author, but noting the deviation from the usual gate.

Three non-blocking findings, all for follow-up rather than this PR:

• PlaceFormPanel.razor — the new "Yes, discard" and "Cancel" confirmation buttons carry no class and will ship as bare browser defaults. That is the exact family task 185 is fixing in a concurrent PR; whichever lands second should pick these up.
• PlaceForm.IsDirty ignores the group selector. In add mode, picking a different group and then dismissing discards that choice with no confirmation.
• PlaceForm.IsDirty reads a cleared description as dirty when the stored value was null — typing into and then emptying the textarea binds "", and "" != null. Harmless (an unnecessary confirmation, never a lost edit), but it will look arbitrary to a reader who did that.

The duplicate-id root cause and the @if (!_closing) + inert fix are right, and better than the click-interception hypothesis they replaced.

Verdict: mergeable Reviewed by the orchestrating agent rather than a separate reviewing subagent — the session's subagent limit is exhausted. Independent of the author, but noting the deviation from the usual gate. Three non-blocking findings, all for follow-up rather than this PR: - `PlaceFormPanel.razor` — the new "Yes, discard" and "Cancel" confirmation buttons carry no class and will ship as bare browser defaults. That is the exact family task 185 is fixing in a concurrent PR; whichever lands second should pick these up. - `PlaceForm.IsDirty` ignores the group selector. In add mode, picking a different group and then dismissing discards that choice with no confirmation. - `PlaceForm.IsDirty` reads a cleared description as dirty when the stored value was `null` — typing into and then emptying the textarea binds `""`, and `"" != null`. Harmless (an unnecessary confirmation, never a lost edit), but it will look arbitrary to a reader who did that. The duplicate-id root cause and the `@if (!_closing)` + `inert` fix are right, and better than the click-interception hypothesis they replaced.

rob merged commit 539c42614c into main 2026-08-11 10:34:03 +00:00

rob deleted branch feat/add-place-panel-polish 2026-08-11 10:34:03 +00:00

rob referenced this pull request from a commit 2026-08-11 10:34:04 +00:00

Animate and dim the add-place panel, fit its fields to the sidebar (#117)

rob referenced this pull request 2026-08-11 10:44:15 +00:00

Give buttons a shared primary/secondary/destructive style #118

rob referenced this pull request 2026-08-11 13:25:46 +00:00

Click an empty part of the map to add a place there #120

rob referenced this pull request 2026-08-12 07:06:16 +00:00

Share one ModalOverlay component across every modal-task panel #137

Sign in to join this conversation.

Reviewers

No reviewers

Labels

Clear labels

No items

No labels

Milestone

Clear milestone

No items

No milestone

Projects

Clear projects

No items

No project

Assignees

Clear assignees

No assignees

1 participant

Notifications

Subscribe

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference

rob/PlaceMark!117

No description provided.