Migrate Breadcrumbs internals to PopoverNext via compat shim#8090
Closed
ggdouglas wants to merge 3 commits into
Closed
Migrate Breadcrumbs internals to PopoverNext via compat shim#8090ggdouglas wants to merge 3 commits into
ggdouglas wants to merge 3 commits into
Conversation
Generate changelog in
|
feat(core): migrate Breadcrumbs to PopoverNext via compat shimBuild artifact links for this commit: documentation | landing | table | demo | storybookThis is an automated comment from the deploy-preview CircleCI job. |
This was referenced Apr 29, 2026
1 task
6bb8a23 to
2556609
Compare
Update compat shim for stricter typingBuild artifact links for this commit: documentation | landing | table | demo | storybookThis is an automated comment from the deploy-preview CircleCI job. |
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.
Context
PopoverNextwas introduced in palantir/blueprint#7573 as the Floating-UI–based replacement for the legacy Popper.js-backedPopover. That PR explicitly defers migrating the components that wrapPopoverinternally and re-expose it via apopoverPropspass-through. Migration utilities and codemods were called out as forthcoming follow-ups.PopoverPropsandPopoverNextPropsare similar but not type-compatible. A handful of props were renamed, a few were dropped, and two defaults shifted. So a direct in-place swap of<Popover>for<PopoverNext>doesn't work: anypopoverPropsbag a consumer passes is typed against the legacy shape, and a few of those legacy fields just don't exist on the new component.This PR migrates Breadcrumbs and lands a reusable shim along the way. Breadcrumbs is the smallest/easiest case, which makes it a good blueprint: the same shim and the same wiring pattern should carry the rest of the deferred component migrations with very little per-component work. The shim is also useful to external consumers migrating their own code.
The aim: swap Breadcrumbs' internal
PopoverforPopoverNextwith no changes to its public API and no observable behavior change for existing consumers.What changed
1. New
popoverPropsToNextProps()migration utilitypackages/core/src/components/popover-next/popoverNextMigrationUtils.tsIt lives next to the two migration helpers already in this file (
popoverPositionToNextPlacement,popperModifiersToNextMiddleware) and reuses them internally. Exported frompackages/core/src/components/index.tsso external consumers can use it for their own migration.2. Breadcrumbs swapped to
PopoverNextpackages/core/src/components/breadcrumbs/breadcrumbs.tsxThe internal overflow-menu
<Popover>is now a<PopoverNext>. Consumer-suppliedpopoverPropsgo throughpopoverPropsToNextProps()before being spread. The shim runs even when nopopoverPropsare passed (popoverProps ?? {}), so the legacy default forshouldReturnFocusOnClose: falsealways applies and consumers don't see a shift.The public
BreadcrumbsPropsinterface is unchanged.popoverPropscontinues to accept the legacyPopoverPropsshape.Prop mapping reference
The legacy
PopoverPropsexposes ~47 props once you include everything inherited fromPopoverSharedProps,OverlayableProps,OverlayLifecycleProps, andProps. The shim handles them in four buckets.children,content,isOpen,defaultIsOpen,disabled,usePortal,interactionKind,popupKind,hasBackdrop,backdropProps,captureDismiss,canEscapeKeyClose,enforceFocus,lazy,transitionDuration,placement,popoverClassName,portalClassName,portalContainer,inheritDarkTheme,matchTargetWidth,hoverOpenDelay,hoverCloseDelay,openOnTargetFocus,autoFocus,fill,targetTagName,targetProps,renderTarget,rootBoundary,positioningStrategy,onClose,onClosed,onClosing,onOpened,onOpening,onInteraction,classNamepositiontoplacementpopoverPositionToNextPlacement(). If bothpositionandplacementare present,placementwins (matches legacy mutex semantics).modifierstomiddlewarepopperModifiersToNextMiddleware()minimal: truetoanimation: "minimal"plusarrow: falseminimalboth uses the subtler animation and disables the arrowboundary: "clippingParents"to"clippingAncestors"ElementandElement[]pass through unchanged.shouldReturnFocusOnClose ?? falsefalse; PopoverNext defaults totrue. Pinned so the swap is invisible.console.warn)modifiersCustommiddleware.popoverRefPopoverNext'sforwardRefexposes{ reposition }, not the popover DOM element.portalStopPropagationEventsNote that
boundaryitself isn't explicitly pinned. PopoverNext's default of"clippingAncestors"is the Floating UI equivalent of legacy's"clippingParents". The strings differ; the semantics don't. Pinning would have been a no-op.Behavior preservation
For Breadcrumbs specifically, the only
popoverPropsfields the existing test suite exercises areisOpenandusePortal, both 1:1. There are no doc-app examples that passpopoverProps. The plausible consumer-visible behavior shifts come down to three spots, and each is mitigated:shouldReturnFocusOnClosedefault. Legacy isfalse, PopoverNext istrue. The shim runs unconditionally and pins it tofalse."clippingParents"and new"clippingAncestors"are the equivalent concept in Floating UI, so no remap is needed when the consumer doesn't supply a boundary.position: "auto". Converts toplacement: undefined, which makes PopoverNext fall back to itsautoPlacementmiddleware. Same end-user behavior as legacy'sflip-based auto.Verification
Manual UI checks worth running before merge (not covered automatically):
Breadcrumbswith overflow and click the collapsed indicator. Default placement:bottom-start.collapseFrom={Boundary.END}should producebottom-end.popoverProps={{ usePortal: false }}: menu mounts inline. This is the path the two skipped tests exercise.popoverProps={{ minimal: true }}: no arrow, minimal animation.popoverProps={{ position: "right" }}: shim converts toplacement: "right".popoverProps={{ modifiersCustom: [] }}in dev mode: console warning fires.Caveats and follow-ups
None of these block Breadcrumbs, but they're worth tracking as a heads-up for the next component migrations that pick up this shim.
The
popoverRefgap is the one I'd flag first. Legacy consumers that reach into the popover DOM viapopoverReflose that capability; PopoverNext's forwarded ref exposes only{ reposition }. If a future component migration runs into a consumer that actually usespopoverRef, that needs a separate fix, probably another ref-forwarding layer in PopoverNext that surfaces the floating element.The
modifiersCustomgap is the same shape: arbitrary Popper.js modifier objects can't be machine-translated into Floating UI middleware. Anything reaching for it has to migrate manually tomiddleware.portalStopPropagationEventsdoesn't work under React 17+ anyway, so dropping it is a no-op in practice. The dev warning exists for discoverability.For the wider migration, the pattern Breadcrumbs follows here is the template: swap
<Popover>for<PopoverNext>and spreadpopoverPropsToNextProps(popoverProps ?? {}). Each future component has its own internalPopovercall to swap, but the public-facingpopoverPropsAPI can stay frozen.