rob/PlaceMark
Watch
1
0
Fork
You've already forked PlaceMark
 0
Code Issues Pull requests Projects Releases Packages 3 Wiki Activity Actions

Dismiss the detail panel on a map click, and rank it below the drawer toggle #184

Merged
rob merged 2 commits from feature/task-244-detail-panel into main 2026-08-17 06:36:01 +00:00
Conversation 3 Commits 2 Files changed 14 +748 -30
rob commented 2026-08-16 21:18:25 +00:00
Owner
Copy link

Closes Vikunja task 244.

Part 1. Home.HandleMapClicked dismissed nothing — a map click always started adding a place (subject only to the eligible-groups check), so clicking empty map while the detail panel was open opened the add-place form on top of it. It now clears the selection first, unconditionally, checked ahead of the eligible-groups guard so a Viewer-only caller — whose EligibleGroupsForAdd is always empty — can still dismiss the panel. A marker click is untouched: it reaches Home through the separate OnMapMarkerActivated callback, which map.js's own bubblingMouseEvents: false already keeps from ever mixing with a genuine map click. map.js itself is not touched, so the deferred-click machinery and the double-tap drag-zoom gesture are unaffected.

Part 2. PlaceDetailPanel shared --z-overlay with ModalOverlay, which is also what let it outrank the drawer's own toggle (calc(var(--z-drawer) + 1), well below 1300). theme.css gains two rungs between --z-drawer and --z-overlay: --z-panel (the panel's new home — still above the drawer, so an open drawer still can't hide it) and --z-header (the toggle's new, dedicated rung, replacing its old computation off --z-drawer). ModalOverlay and ImportReportBanner are untouched, so a modal-task panel still covers the toggle while it holds the focus trap — the obvious fix (raising the toggle above --z-overlay instead) would have broken that.

ADR-0156 records both, partially superseding ADR-0105 (the unconditional half of its acceptance criterion) and ADR-0059 (the toggle's old z-index computation, and its now-stale "not consumed by anything yet" claim about --z-overlay/--z-dialog).

Testing. bUnit (HomeTests) fully expresses Part 1's state machine — dismissal, its ordering ahead of the eligible-groups guard, and that a marker click and the belt-and-braces editing case are untouched — through the same OnMapAreaClicked/OnMapMarkerActivated seam already used elsewhere in that class. ZIndexLadderTests (new, PlaceMark.Architecture.Tests) statically proves the ladder's declared order and that each selector references the right token. Neither can prove Part 2's actual point — bUnit computes no style at all — so ZIndexComputedStackingJourneyTests (E2E) reads real getComputedStyle in Chromium for both directions: panel below the toggle, and a modal still above it (the assertion that catches the wrong fix). MapClickDismissesDetailPanelJourneyTests (E2E) adds the one thing bUnit can't: a genuine mouse drag ending over empty map must not be read as a click and dismiss the panel.

Every new test was watched fail first against the real mistake it guards, not merely reasoned about — including, for the two E2E classes, a false pass caught mid-review where a stale incremental build (traced to a git stash/pop cycle) served old CSS and old Home.razor.cs behaviour despite current source on disk; a dotnet clean and rebuild was needed before the reverted-code run genuinely reddened. Full solution dotnet test at the repo root passes (Domain 12, Contracts 139, Architecture 128, WebUI 817, Api 607, Infrastructure 321 — E2E skipped by default, opt-in via PLACEMARK_RUN_E2E_TESTS=true, and the four new + all touched-adjacent E2E tests verified manually against a real browser and app instance both with and without the fix).

Closes Vikunja task 244. **Part 1.** `Home.HandleMapClicked` dismissed nothing — a map click always started adding a place (subject only to the eligible-groups check), so clicking empty map while the detail panel was open opened the add-place form on top of it. It now clears the selection first, unconditionally, checked ahead of the eligible-groups guard so a Viewer-only caller — whose `EligibleGroupsForAdd` is always empty — can still dismiss the panel. A marker click is untouched: it reaches `Home` through the separate `OnMapMarkerActivated` callback, which `map.js`'s own `bubblingMouseEvents: false` already keeps from ever mixing with a genuine map click. `map.js` itself is not touched, so the deferred-click machinery and the double-tap drag-zoom gesture are unaffected. **Part 2.** `PlaceDetailPanel` shared `--z-overlay` with `ModalOverlay`, which is also what let it outrank the drawer's own toggle (`calc(var(--z-drawer) + 1)`, well below 1300). `theme.css` gains two rungs between `--z-drawer` and `--z-overlay`: `--z-panel` (the panel's new home — still above the drawer, so an open drawer still can't hide it) and `--z-header` (the toggle's new, dedicated rung, replacing its old computation off `--z-drawer`). `ModalOverlay` and `ImportReportBanner` are untouched, so a modal-task panel still covers the toggle while it holds the focus trap — the obvious fix (raising the toggle above `--z-overlay` instead) would have broken that. ADR-0156 records both, partially superseding ADR-0105 (the unconditional half of its acceptance criterion) and ADR-0059 (the toggle's old z-index computation, and its now-stale "not consumed by anything yet" claim about `--z-overlay`/`--z-dialog`). **Testing.** bUnit (`HomeTests`) fully expresses Part 1's state machine — dismissal, its ordering ahead of the eligible-groups guard, and that a marker click and the belt-and-braces editing case are untouched — through the same `OnMapAreaClicked`/`OnMapMarkerActivated` seam already used elsewhere in that class. `ZIndexLadderTests` (new, `PlaceMark.Architecture.Tests`) statically proves the ladder's declared order and that each selector references the right token. Neither can prove Part 2's actual point — bUnit computes no style at all — so `ZIndexComputedStackingJourneyTests` (E2E) reads real `getComputedStyle` in Chromium for both directions: panel below the toggle, and a modal still above it (the assertion that catches the wrong fix). `MapClickDismissesDetailPanelJourneyTests` (E2E) adds the one thing bUnit can't: a genuine mouse drag ending over empty map must not be read as a click and dismiss the panel. Every new test was watched fail first against the real mistake it guards, not merely reasoned about — including, for the two E2E classes, a false pass caught mid-review where a stale incremental build (traced to a `git stash`/`pop` cycle) served old CSS and old `Home.razor.cs` behaviour despite current source on disk; a `dotnet clean` and rebuild was needed before the reverted-code run genuinely reddened. Full solution `dotnet test` at the repo root passes (Domain 12, Contracts 139, Architecture 128, WebUI 817, Api 607, Infrastructure 321 — E2E skipped by default, opt-in via `PLACEMARK_RUN_E2E_TESTS=true`, and the four new + all touched-adjacent E2E tests verified manually against a real browser and app instance both with and without the fix).
Dismiss the detail panel on a map click, and rank it below the drawer toggle
All checks were successful
CI / build (pull_request) Successful in 5m34s
Details
CI / container-images (pull_request) Successful in 4s
Details
CI / e2e (pull_request) Successful in 3m25s
Details
55d8bca6ba 
A map click now clears the selection when the detail panel is open, checked
ahead of the eligible-groups guard so a Viewer-only caller can still dismiss
it, instead of always starting the add-place flow underneath it. The panel
moves onto a new --z-panel rung below the drawer's own toggle (--z-header),
which no longer computes its own position off --z-drawer; ModalOverlay stays
at --z-overlay so a modal-task panel still covers the toggle while it holds
the focus trap.
rob reviewed 2026-08-16 21:35:34 +00:00
rob left a comment
Copy link

Verdict: changes needed

Part 2 likely fixes only the toggle, not "the main menu". NavMenu renders inside <aside class="drawer"> (Drawer.razor), not as separate header chrome — NavMenu.razor.css's own comment says as much ("the nav now lives in the drawer"). .drawer stays at --z-drawer (1200), unchanged and still below --z-panel (1250) — exactly the same relative order it had against the panel before this PR (previously drawer 1200 vs panel at --z-overlay 1300, also below). The only element whose rank against the panel actually changed is .drawer-toggle, raised from 1201 to --z-header (1275).

So: when the drawer is open (default on desktop, or after tapping the toggle on mobile) and the detail panel is showing, the panel still paints over the drawer's own visible content — NavMenu's nav links, PlaceAccordion, "Add group" — on any viewport narrow enough for the two to overlap (--drawer-width: min(20rem, 85vw) vs the panel's own min(20rem, calc(100vw - 2 * var(--space-medium))) overlap significantly below the 64rem breakpoint). If Rob's report ("renders above the main menu") was about that open-drawer content rather than the toggle button specifically, this is unfixed and will resurface. Neither ZIndexComputedStackingJourneyTests nor ZIndexLadderTests asserts anything about .drawer vs .place-detail-panel — only toggle vs panel — so this gap is untested either way.

Given ADR-0059 already deliberately placed the panel above the drawer (so an open drawer can't hide it) before this ticket, it's plausible the toggle really is all Rob meant — but ADR-0156 doesn't address the distinction or say why the open drawer's own nav content staying under the panel is fine. Worth confirming with Rob, or adding a line to ADR-0156's Context/Consequences making the scope decision explicit and pinning it with a test, before merging.

Everything else checked out: Part 1's dismissal check runs ahead of the eligible-groups guard (Home.razor.cs), and HomeTests covers the Viewer-only case directly. ModalOverlay/ImportReportBanner are untouched at --z-overlay/--z-dialog, and ZIndexComputedStackingJourneyTests does assert a real modal above the toggle, not just the panel below it. map.js is untouched, and the drag-survives-dismissal E2E test drives a real Playwright mouse down/move/up sequence rather than synthetic events. Both ADR annotations quote ADR-0059's and ADR-0105's frozen text accurately, and both stay Accepted per ADR-0107.

Verdict: changes needed **Part 2 likely fixes only the toggle, not "the main menu".** `NavMenu` renders inside `<aside class="drawer">` (`Drawer.razor`), not as separate header chrome — `NavMenu.razor.css`'s own comment says as much ("the nav now lives in the drawer"). `.drawer` stays at `--z-drawer` (1200), unchanged and still below `--z-panel` (1250) — exactly the same relative order it had against the panel before this PR (previously drawer 1200 vs panel at `--z-overlay` 1300, also below). The only element whose rank against the panel actually changed is `.drawer-toggle`, raised from 1201 to `--z-header` (1275). So: when the drawer is open (default on desktop, or after tapping the toggle on mobile) and the detail panel is showing, the panel still paints over the drawer's own visible content — `NavMenu`'s nav links, `PlaceAccordion`, "Add group" — on any viewport narrow enough for the two to overlap (`--drawer-width: min(20rem, 85vw)` vs the panel's own `min(20rem, calc(100vw - 2 * var(--space-medium)))` overlap significantly below the 64rem breakpoint). If Rob's report ("renders above the main menu") was about that open-drawer content rather than the toggle button specifically, this is unfixed and will resurface. Neither `ZIndexComputedStackingJourneyTests` nor `ZIndexLadderTests` asserts anything about `.drawer` vs `.place-detail-panel` — only toggle vs panel — so this gap is untested either way. Given ADR-0059 already deliberately placed the panel above the drawer (so an open drawer can't hide it) before this ticket, it's plausible the toggle really is all Rob meant — but ADR-0156 doesn't address the distinction or say why the open drawer's own nav content staying under the panel is fine. Worth confirming with Rob, or adding a line to ADR-0156's Context/Consequences making the scope decision explicit and pinning it with a test, before merging. Everything else checked out: Part 1's dismissal check runs ahead of the eligible-groups guard (`Home.razor.cs`), and `HomeTests` covers the Viewer-only case directly. `ModalOverlay`/`ImportReportBanner` are untouched at `--z-overlay`/`--z-dialog`, and `ZIndexComputedStackingJourneyTests` does assert a real modal above the toggle, not just the panel below it. `map.js` is untouched, and the drag-survives-dismissal E2E test drives a real Playwright mouse down/move/up sequence rather than synthetic events. Both ADR annotations quote ADR-0059's and ADR-0105's frozen text accurately, and both stay `Accepted` per ADR-0107.
src/PlaceMark.WebUI/Layout/Drawer.razor.css Outdated
@ -17,0 +14,4 @@
/* --z-header (task 244, ADR-0156), not calc(var(--z-drawer) + 1) any more — that computation only
ever guaranteed this sat above the drawer itself, and PlaceDetailPanel (--z-panel, above
--z-drawer but below this) was still free to bury it. A dedicated rung is what actually keeps
the promise this comment already made before ADR-0156: the toggle a reader reaches for is never
rob commented 2026-08-16 21:35:34 +00:00
Author
Owner
Copy link

Only .drawer-toggle moved to --z-header. .drawer itself (below in this file) stays at --z-drawer, still below --z-panel — same relationship to the panel it had before this PR. When the drawer is open and the panel is showing, the panel still paints over NavMenu/PlaceAccordion inside it.

Only .drawer-toggle moved to --z-header. `.drawer` itself (below in this file) stays at --z-drawer, still below --z-panel — same relationship to the panel it had before this PR. When the drawer is open and the panel is showing, the panel still paints over NavMenu/PlaceAccordion inside it.
src/PlaceMark.WebUI/wwwroot/css/theme.css Outdated
@ -122,0 +121,4 @@
--z-drawer: the drawer's own sliding <aside> (Drawer.razor.css's own ".drawer"). Deliberately
below --z-panel, not above it, so a browsing panel that needs to stay reachable over an open
drawer on a narrow viewport (--z-panel's own reason for existing) can — the property ADR-0059
rob commented 2026-08-16 21:35:34 +00:00
Author
Owner
Copy link

This confirms the panel-above-drawer relationship is unchanged by this PR: --z-panel (1250) > --z-drawer (1200), same order as when the panel sat at the old --z-overlay (1300). Only --z-header (the toggle) changed rank relative to the panel. If Rob's "above the main menu" report meant the open drawer's own nav content (NavMenu/PlaceAccordion), not just the toggle button, this doesn't fix it.

This confirms the panel-above-drawer relationship is unchanged by this PR: --z-panel (1250) > --z-drawer (1200), same order as when the panel sat at the old --z-overlay (1300). Only --z-header (the toggle) changed rank relative to the panel. If Rob's "above the main menu" report meant the open drawer's own nav content (NavMenu/PlaceAccordion), not just the toggle button, this doesn't fix it.
rob force-pushed feature/task-244-detail-panel from 55d8bca6ba
All checks were successful
CI / build (pull_request) Successful in 5m34s
Details
CI / container-images (pull_request) Successful in 4s
Details
CI / e2e (pull_request) Successful in 3m25s
Details
to 2b83be61e1
All checks were successful
CI / build (pull_request) Successful in 4m55s
Details
CI / container-images (pull_request) Successful in 3s
Details
CI / e2e (pull_request) Successful in 5m58s
Details
2026-08-17 06:19:49 +00:00 Compare
rob commented 2026-08-17 06:20:05 +00:00
Author
Owner
Copy link

Fixed in 2b83be6. .drawer now shares --z-header with .drawer-toggle instead of sitting at its own --z-drawer rung — removed --z-drawer outright once nothing referenced it. Drawer's OverlayBackdrop (BelowZIndexToken) moved from --z-drawer to --z-header alongside it, so its scrim now dims the panel too while the drawer is open.

Added the direct assertion you asked for: PlaceDetailPanel_And_Drawer_DeclaredZIndex_RanksThePanelBelowTheDrawer (ZIndexLadderTests, static) and DetailPanelOpen_ComputedZIndex_IsBelowTheOpenDrawersOwnAsideToo (E2E, real getComputedStyle). Both watched fail first against the toggle-only fix — the E2E one against a real running instance, dotnet cleand first.

ADR-0156 rewritten (and renamed, since it wasn't merged yet) to record the reversal of ADR-0059's "never hidden behind an open drawer" placement as a deliberate Consequence, not a detail. ADR-0059's own annotation now names that placement, not just the toggle's z-index computation, as part of what's superseded.

Branch rebased onto current main (PR #185). Full solution dotnet test green; full E2E suite (33) green, including the two new tests and everything from tasks 241–243/246.

Fixed in 2b83be6. `.drawer` now shares `--z-header` with `.drawer-toggle` instead of sitting at its own `--z-drawer` rung — removed `--z-drawer` outright once nothing referenced it. `Drawer`'s `OverlayBackdrop` (`BelowZIndexToken`) moved from `--z-drawer` to `--z-header` alongside it, so its scrim now dims the panel too while the drawer is open. Added the direct assertion you asked for: `PlaceDetailPanel_And_Drawer_DeclaredZIndex_RanksThePanelBelowTheDrawer` (`ZIndexLadderTests`, static) and `DetailPanelOpen_ComputedZIndex_IsBelowTheOpenDrawersOwnAsideToo` (E2E, real `getComputedStyle`). Both watched fail first against the toggle-only fix — the E2E one against a real running instance, `dotnet clean`d first. ADR-0156 rewritten (and renamed, since it wasn't merged yet) to record the reversal of ADR-0059's "never hidden behind an open drawer" placement as a deliberate Consequence, not a detail. ADR-0059's own annotation now names that placement, not just the toggle's z-index computation, as part of what's superseded. Branch rebased onto current `main` (PR #185). Full solution `dotnet test` green; full E2E suite (33) green, including the two new tests and everything from tasks 241–243/246.
rob reviewed 2026-08-17 06:35:33 +00:00
rob left a comment
Copy link

Verdict: mergeable

Re-reviewed the layering change at 2b83be61.

  1. --z-drawer removal is clean. It was only ever referenced by .drawer and .drawer-toggle in Drawer.razor.css, both now on --z-header; no other stylesheet in the tree referenced it. ZIndexLadderTests' ladder is now four rungs (--z-panel, --z-header, --z-overlay, --z-dialog) with its ordering test and comments updated to match — no gap left inviting a stray number.

  2. OverlayBackdrop's BelowZIndexToken move checks out. .overlay-backdrop's z-index: calc(var(--overlay-backdrop-below-token) - 1) puts the drawer's own scrim at 1274 — above --z-panel (1250, so it now dims the open panel too, as ADR-0156 says explicitly) and below --z-header (1275, so it never covers the drawer's own content). ModalOverlay/ImportReportBanner are untouched at --z-overlay/--z-dialog, and the modal backdrop (still defaulting to --z-overlay) resolves to 1299 — above --z-header, so a modal still covers the drawer and its toggle.

  3. ADR-0156's rewrite is sound. Both quoted displacements from ADR-0059 ("calc(var(--z-drawer) + 1)", "not consumed by anything yet") match the frozen original verbatim, plus the new, honestly-stated third displacement (the panel's deliberate placement above the drawer, now reversed). ADR-0059 and ADR-0105 both stay Accepted, satisfying ADR-0107's partial-supersession shape. The rename is fine — this file was never merged under its old name, so nothing frozen was touched. Both ADR-0059's and ADR-0105's Partially superseded by links, and the README index row, all point at the new filename correctly.

  4. The new E2E assertion is the right one. DetailPanelOpen_ComputedZIndex_IsBelowTheOpenDrawersOwnAsideToo reads computed z-index on aside.drawer itself — the real open drawer element carrying NavMenu/PlaceAccordion — not .drawer-toggle. That's genuinely the assertion this needed; a fix raising only the toggle would still fail it.

  5. Everything cleared last time is undisturbed — Home.razor.cs, HomeTests.cs and MapClickDismissesDetailPanelJourneyTests.cs are identical to the version already reviewed (same diffs), map.js remains untouched, and the Playwright drag test is unchanged.

Verdict: mergeable Re-reviewed the layering change at `2b83be61`. 1. **`--z-drawer` removal is clean.** It was only ever referenced by `.drawer` and `.drawer-toggle` in `Drawer.razor.css`, both now on `--z-header`; no other stylesheet in the tree referenced it. `ZIndexLadderTests`' ladder is now four rungs (`--z-panel`, `--z-header`, `--z-overlay`, `--z-dialog`) with its ordering test and comments updated to match — no gap left inviting a stray number. 2. **`OverlayBackdrop`'s `BelowZIndexToken` move checks out.** `.overlay-backdrop`'s `z-index: calc(var(--overlay-backdrop-below-token) - 1)` puts the drawer's own scrim at 1274 — above `--z-panel` (1250, so it now dims the open panel too, as ADR-0156 says explicitly) and below `--z-header` (1275, so it never covers the drawer's own content). `ModalOverlay`/`ImportReportBanner` are untouched at `--z-overlay`/`--z-dialog`, and the modal backdrop (still defaulting to `--z-overlay`) resolves to 1299 — above `--z-header`, so a modal still covers the drawer and its toggle. 3. **ADR-0156's rewrite is sound.** Both quoted displacements from ADR-0059 ("`calc(var(--z-drawer) + 1)`", "not consumed by anything yet") match the frozen original verbatim, plus the new, honestly-stated third displacement (the panel's deliberate placement above the drawer, now reversed). ADR-0059 and ADR-0105 both stay `Accepted`, satisfying ADR-0107's partial-supersession shape. The rename is fine — this file was never merged under its old name, so nothing frozen was touched. Both ADR-0059's and ADR-0105's `Partially superseded by` links, and the README index row, all point at the new filename correctly. 4. **The new E2E assertion is the right one.** `DetailPanelOpen_ComputedZIndex_IsBelowTheOpenDrawersOwnAsideToo` reads computed `z-index` on `aside.drawer` itself — the real open drawer element carrying `NavMenu`/`PlaceAccordion` — not `.drawer-toggle`. That's genuinely the assertion this needed; a fix raising only the toggle would still fail it. 5. **Everything cleared last time is undisturbed** — `Home.razor.cs`, `HomeTests.cs` and `MapClickDismissesDetailPanelJourneyTests.cs` are identical to the version already reviewed (same diffs), `map.js` remains untouched, and the Playwright drag test is unchanged.
rob merged commit fe5fec734b into main 2026-08-17 06:36:01 +00:00
rob deleted branch feature/task-244-detail-panel 2026-08-17 06:36:01 +00:00
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
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!184
No description provided.