Replace BPopover with custom GPopover component - #21959
Open
dannon wants to merge 14 commits into
Open
Conversation
9 tasks
dannon
force-pushed
the
feature/g-popover
branch
from
March 24, 2026 02:12
571d542 to
016c5ae
Compare
itisAliRH
reviewed
Mar 24, 2026
Member
There was a problem hiding this comment.
I couldn’t find any references to this component. If it’s no longer used, we should consider removing it.
itisAliRH
requested changes
Mar 24, 2026
itisAliRH
left a comment
itisAliRH
left a comment
Member
There was a problem hiding this comment.
Overall, this looks good to me, thank you @dannon. While testing it, I noticed a few issues that should be addressed:
1- Popover UX differences vs. BPopover:
- The arrow pointing to the reference element is missing. Visually, it was helpful and I think we should keep it.
- The show/hide behavior is not smooth and lacks animation (this could be handled in a follow-up).
- More importantly, there is a usability issue: when moving the cursor from the trigger to the popover, it often closes because there is a small gap where the cursor leaves the hover area. You have to move very quickly to keep it open (I suspect reintroducing the arrow (or otherwise bridging that gap) would help):
21959.-.1.mov
2- Workflow Editor readability issue:
- In the Workflow Editor, when clicking on the organization/person icon, the displayed table is hard to read (layout/spacing issue):
|
|
|
dannon
force-pushed
the
feature/g-popover
branch
from
March 30, 2026 12:48
016c5ae to
3ac8398
Compare
dannon
force-pushed
the
feature/g-popover
branch
from
April 2, 2026 14:29
3ac8398 to
8e5c4ab
Compare
mvdbeek
marked this pull request as draft
April 7, 2026 12:27
Member
|
This needs a rebase |
Member
First issue already fixed👍🏻 The organization/person icon popover in the Workflow Editor is not working anymore; it's also broken on dev, so it might not be related to these changes. |
dannon
force-pushed
the
feature/g-popover
branch
from
July 30, 2026 03:23
c997187 to
48afd56
Compare
dannon
force-pushed
the
feature/g-popover
branch
from
August 15, 2026 01:17
ddabc61 to
151eadb
Compare
dannon
marked this pull request as ready for review
August 20, 2026 14:50
dannon
force-pushed
the
feature/g-popover
branch
from
August 24, 2026 01:17
151eadb to
1f30c71
Compare
Add GPopover as a Bootstrap-Vue-free popover using @floating-ui/dom for positioning, following the same pattern as the existing GTooltip. Supports hover/click/manual triggers, programmatic show/hide via :show.sync, title and content via props or slots, placement mapping from BPopover strings to floating-ui equivalents, and boundary awareness via altBoundary. Migrated all 15 files that used BPopover, including dynamic component switching (FilterMenu, FormDataExtensions), manual trigger control (WorkflowAttributes best practice popovers), and click+blur triggers (SchemaOrg viewers).
Find GPopover component directly instead of by BPopover name, and check the target via props() instead of attributes() since GPopover doesn't render the target as a DOM attribute.
Widen target prop from HTMLElement to Element (floating-ui accepts Element, and FontAwesomeIcon renders SVG). Allow function targets to return any value matching BPopover's behavior — resolveTarget handles Vue component instances via . unwrapping. Fix String -> string in three TemplateSummaryPopover wrappers.
Adds the Floating UI arrow() middleware so the popover renders a CSS arrow pointing at its reference element, matching the old BPopover behavior. Uses Bootstrap 4's .arrow class for styling compatibility. Also fixes the hover gap problem where moving the mouse from trigger to popover would close it -- the 10px offset created a dead zone. Now uses a 100ms deferred hide that gets cancelled if the mouse enters the popover in time. Tracks actualPlacement from Floating UI so the arrow direction stays correct when the popover flips.
BPopover rendered at body level, so the global .popover { max-width: 70% }
rule resolved against the viewport. GPopover was rendering inline, which
meant inside narrow containers like the ActivityPanel the effective width
shrank to ~210px, crushing tables and other wide content.
GPopover uses update:show (bound via :show.sync), not update:modelValue, so the v-model binding was a no-op. Also removed a stale commented-out emit.
Noticed during PR review -- the component had no references anywhere in the codebase. It got picked up in the GPopover migration since it was still checked in, but it's dead code.
…owExtractionMessages Catches up the sweep with files added on dev after the branch was cut -- mostly davelopez's StorageOperations batch plus a TODO in WorkflowExtractionMessages that was waiting on GPopover. Same drop-in swap as the others. Also broadens GPopover's TriggerType to accept the 'manual hover' combination BPopover supports (used by HistoryStorageOperationsIndicator for proactive auto-show with hover backup).
The arrow wasn't drawing a triangle on most call sites. The template emitted
`bs-popover-${actualPlacement}` straight from floating-ui, so it produced things
like bs-popover-bottom-start, but Bootstrap only defines .bs-popover-top,
-right, -bottom and -left and hangs every arrow triangle rule off those four.
The aligned variants matched no rule at all, so the .arrow div rendered with no
borders on it. Six of the thirteen placements in the codebase are compound BV
strings (bottomleft, topleft, rightbottom) that map to -start/-end, so the arrow
was silently missing on all of them -- including both object store popovers and
FilterMenu's popover view. Collapsing the class to its base side brings it back.
Also zeroed Bootstrap's horizontal margin on .arrow. The arrow middleware
already centers it on the reference element, and the inherited margin was
shifting it back off-center by the popover border radius.
Vue 2.7 has no built-in Teleport, and it doesn't warn about one either -- the bare <Teleport to="body"> was just an unknown element, so the popover rendered inside it, inline, still subject to every ancestor overflow, transform and stacking context it was supposed to escape. Swapped it for Vue2Teleport, which the repo already depends on and already uses in HeadlessMultiselect, and added a test asserting the popover ends up a direct child of body. Confirmed in a browser against the workflow editor: six popovers now sit directly under body and there are no leftover <teleport> elements in the DOM.
PersonViewer and OrganizationViewer were passing `$refs['button'] || 'works-lazily'` as the popover target. $refs is empty during the first render, so the target was the junk string, and since Vue 2's $refs isn't reactive nothing ever re-triggered resolution -- the popover couldn't open at all until some unrelated change forced the component to re-render. The hack predates the GPopover migration and it's the reason clicking the person or organization icon in the workflow editor so often did nothing. Give each icon a page-unique id from useUid and target that instead, which is how the other eleven GPopover call sites already work. Tests cover both viewers: the target has to resolve to an element that's really in the document, and two instances have to get distinct ids, since a workflow can list several creators. Verified in the workflow editor -- both popovers now open on the first click with no re-render.
Clicking a creator in the workflow editor gave you a 70px-wide popover with a 290px table spilling out of it. The cause is that GTable sets container-type: inline-size so it can drive its own container queries, and inline-size containment makes an element's width independent of its contents -- so the table reported no intrinsic width at all. The popover is shrink-to-fit, so the only thing left contributing any width was the title, and everything else overflowed. Measured with max-content, the whole popover asked for 70px. Opting out of containment inside a popover gets the box back to a sensible 276px, and letting long unbroken values wrap brings the table down to 250px so it fits with room to spare instead of overflowing by 14px. Both are needed -- the wrap alone leaves the box at 70px and makes it 1033px tall. The tradeoff is that GTable's responsive stacked layout won't kick in inside a popover, which seems like the right call: a popover is already a small fixed box and the stacked layout is meant for genuinely narrow viewports. No unit test -- happy-dom doesn't apply SFC styles, so getComputedStyle returns nothing and there's nothing for vitest to assert on. Verified by measuring the live workflow editor: person and organization popovers both 276px wide with a 250px table, no overflow, no horizontal scroll, no clipped cells, and text-only popovers unchanged at 250px.
The comment above it said PersonViewer uses a BPopover that doesn't work in the test environment, which stopped being true once this branch migrated PersonViewer and OrganizationViewer to GPopover. Checked whether the suppression was still load-bearing by neutralising it and running the suite -- all four tests still pass, and vitest-fail-on-console would have caught any warning it was hiding. So the call goes too, not just the stale comment. The helper itself stays; three other test files still use it.
dannon
force-pushed
the
feature/g-popover
branch
from
August 26, 2026 17:35
1f30c71 to
a44af6a
Compare
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
3 participants
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.
Part of the bootstrap-vue scoped-slot replacement effort tracked in #21956.
BPopoveruses scoped slots and crashes under@vue/compat.GPopoveris a custom implementation using@floating-ui/dom(already a project dependency) for positioning, with support for hover/click/manual trigger modes, placement with auto-flip/shift, title and content via props or slots, andcustomClass.Migrates 15 files. Uses Bootstrap's existing popover CSS classes for styling compatibility — custom styles can replace them when Bootstrap CSS is eventually dropped (noted in component docblock).