refactor: apparat popover - #299
Open
domsteinbach wants to merge 1 commit into
Open
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
Refactors the “Apparat” popover behavior in the Fassungen page to support multiple pinned popovers alongside a single transient hover popover, while improving positioning logic and addressing an accessibility/CSS scoping warning.
Changes:
- Split popover state into one transient hover popover (
apparatHover) and multiple pinned popovers (apparatPinned), with page-level Escape handling. - Updated popover positioning to prefer
bottomplacement with configurable flip fallbacks per column. - Adjusted focus behavior so only pinned (click-opened) popovers autofocus; refined global CSS selector to avoid Svelte warning.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/routes/fassungen/[thirties=thirties]/ApparatPopover.svelte | Updates Floating UI placement/flip behavior, adds optional autofocus action, and fixes global selector usage. |
| src/routes/fassungen/[thirties=thirties]/+page.svelte | Refactors popover state/events to support pinned + hover popovers, adds Escape dismiss logic, and computes per-column fallback placement. |
Suppressed comments (1)
src/routes/fassungen/[thirties=thirties]/+page.svelte:376
onMouseEnterApparatTriggeralso relies onev.target. If the trigger anchor contains nested elements,ev.targetcan be that nested element and the transient popover will attach to the wrong node (or have empty content). Useev.currentTargetto reliably read the anchor’sdata-*attributes and compute its position.
const onMouseEnterApparatTrigger = (/** @type { Event } */ ev) => {
clearTimeouts();
if (!(ev.target instanceof HTMLElement)) return;
// A pinned popover already covers this anchor; no transient one needed.
if (apparatPinned.some((p) => p.elTrigger === ev.target)) return;
apparatHover = readApparatData(ev.target);
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
361
to
369
| const onClickApparatTrigger = (/** @type { Event } */ ev) => { | ||
| ignoreApparatLeave = true; | ||
| if (ev.target instanceof HTMLElement) { | ||
| fillApparatStore(ev.target, false); | ||
| if (!(ev.target instanceof HTMLElement)) return; | ||
| clearTimeouts(); | ||
| // The pinned popover supersedes the transient one for this trigger. | ||
| if (apparatHover?.elTrigger === ev.target) apparatHover = undefined; | ||
| // Don't pin the same anchor twice. | ||
| if (!apparatPinned.some((p) => p.elTrigger === ev.target)) { | ||
| apparatPinned.push(readApparatData(ev.target)); | ||
| } |
Comment on lines
+633
to
+639
| <ApparatPopover | ||
| resetPopup={closeApparatOnInteraction} | ||
| onMouseEnter={onMouseEnterApparatPopover} | ||
| onMouseLeave={onMouseLeaveApparatPopover} | ||
| elTrigger={apparatHover.elTrigger} | ||
| fallback={apparatFallback(apparatHover.elTrigger)} | ||
| dreissiger={apparatHover.dreissiger} |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR fixes some bugs, adjusts the positioning of the popover and gets rid of a warning.
A) fixes:
Pinning a popover does not make sense when we can not pin several popovers at once by click or deactivating other popovers.
How it should be instead:
How it should be instead:
A unpinned popover should be destroyed when hovering over another anchor/trigger. A new popover should appear at the right position.
B) Positioning of the popovers
The placement was always on top of the anchor which overlapped the content/text before.
Now it is always below the triggering anchor - except when there is not enough space below. In that case it switches the position
C) get rid of a warning, make better accessible
.fassungen_popover :global(.note) resulted in a warning as there was no component in scope.
changed to { into :global(.fassungen_popover .note) { does not throw a warning but keeps the style the same.