Conversation
…gger Now that `Floating` resolves the reference (anchor) element from the DOM, `PopoverAnchor` no longer needs any ref plumbing. This removes it: - `PopoverAnchor` becomes a plain function component. It was a `React.forwardRef` component purely to receive the anchor ref, and nothing passes it a ref anymore after WB-2119.2. - The hand-rolled `mergeRefs` helper is deleted. It existed to merge the anchor ref with the trigger's own ref; there is no anchor ref to merge, so a trigger's `ref` is now left untouched. - The function-as-children and element-children branches are unified, since the only thing that differed between them was the ref handling. The user-visible effect is that a `Popover` trigger can be of any component type, including a plain function component, and never has to accept or forward a ref. It only has to spread the props it is given onto its own element (which it needs to do anyway for the interaction and ARIA props). Issue: WB-2119 Automated: `pnpm typecheck`, `pnpm lint`, and `pnpm build` all pass. The Popover jest suite passes, with new coverage for a plain function component as the trigger and for a trigger's own ref being left alone. Manual (Storybook — Popover stories): verify the popover still anchors and positions correctly for the element-children and function-as-children trigger patterns, and that the Tabs `TabLabelRenderFunction` story (popover anchored to a tab) still positions and behaves correctly. Please review these risky changes 1. 🔷 `packages/wonder-blocks-popover/src/components/popover-anchor.tsx`: Changes the contract between `Popover` and the trigger element it wraps.
🦋 Changeset detectedLatest commit: 98da780 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
jandrade
commented
Aug 28, 2026
| // the popover against it. | ||
| [FloatingReferenceAttributeName]: floatingReferenceId, | ||
| } as const; | ||
| export default function PopoverAnchor(props: Props) { |
Member
Author
There was a problem hiding this comment.
🔷 This is the file to review in this PR — it changes the contract between Popover and the trigger element it wraps.
After #3163, Floating resolves the reference (anchor) element from the DOM via the attribute it injects, and nothing passes a ref into PopoverAnchor any more. So the ref plumbing that was left behind is removed here:
React.forwardRef→ a plain function component. It was aforwardRefcomponent purely to receive the anchor ref.- The hand-rolled
mergeRefshelper is deleted. It existed to merge the anchor ref with the trigger's own ref; there is no anchor ref to merge, so a trigger'srefis now left untouched rather than being wrapped. - The function-as-children and element-children branches are unified, since ref handling was the only thing that differed between them.
The user-visible consequence, and the reason this is its own PR + changeset: a Popover trigger can now be any component type — host element, forwardRef component, class component, or plain function component — and never has to accept or forward a ref. It only has to spread the props it is given onto its own element, which it needs to do anyway for the interaction and ARIA props.
Contributor
|
Size Change: -116 B (-0.09%) Total Size: 133 kB 📦 View Changed
ℹ️ View Unchanged
|
Contributor
npm Snapshot: Published🎉 Good news!! We've packaged up the latest commit from this PR (ac0b00b) and published all packages with changesets to npm. You can install the packages in ./dev/tools/deploy_wonder_blocks.js --tag="PR3189"Packages can also be installed manually by running: pnpm add @khanacademy/wonder-blocks-<package-name>@PR3189 |
Contributor
A new build was pushed to Chromatic! 🚀https://5e1bf4b385e3fb0020b7073c-qowurnjpqv.chromatic.com/ Chromatic results:
|
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
1 participant
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.
Summary:
Now that
Floatingresolves the reference (anchor) element from the DOM, thepopover's anchor no longer needs any ref plumbing. This PR removes it.
This is the follow-up to #3163, which did the
Popover→Floatingmigration.That PR made the one change to
PopoverAnchorneeded for the popover toposition at all — spreading the
FloatingReferenceAttributeNameattributethrough to the trigger — and left the rest alone, so this PR is a small, focused
cleanup on top of it.
PopoverAnchorbecomes a plain function component. It was aReact.forwardRefcomponent purely to receive the anchor ref, and after🚨 Refactor Popover to use wonder-blocks-floating #3163 nothing passes it a ref anymore.
mergeRefshelper is deleted. It existed to merge the anchorref with the trigger's own ref; there is no anchor ref to merge, so a
trigger's
refis now left untouched.handling was the only thing that differed between them.
The user-visible effect is the reason this is a separate PR: a
Popovertriggercan now be of any component type — host element,
React.forwardRefcomponent, class component, or plain function component — and never has to
accept or forward a ref. It only has to spread the props it is given onto its
own element (which it needs to do anyway for the interaction and ARIA props).
Issue: WB-2119
Test plan:
Automated:
pnpm typecheck,pnpm lint, andpnpm buildall pass. The Popoverjest suite passes, with new coverage for a plain function component as the
trigger and for a trigger's own ref being left alone.
Manual (Storybook — Popover stories):
element-children trigger pattern.
TabLabelRenderFunctionstory (popover anchored to a tab)still positions and behaves correctly.
Review plan:
Please review these risky changes
popover-anchor.tsxCommon patterns:
1 file: Drop
forwardRef+mergeRefs; the trigger is identified by theattribute
Floatinginjects rather than by a ref.