[WEB-4683] feat: popover component added to propel package#7607
[WEB-4683] feat: popover component added to propel package#7607sriramveeraghanta merged 1 commit intopreviewfrom
Conversation
WalkthroughAdds a new popover module. Introduces a Popover compound component wrapping BasePopover with typed placement mapping and content/trigger wrappers. Exposes a barrel index re-exporting the popover root. Provides placement-to-side/align conversion, optional container targeting, and memoized components for trigger, portal, positioner, and content. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor U as User
participant TR as Popover.Trigger
participant PR as Popover.Root
participant CT as Popover.Content
participant PT as Portal
participant PS as Positioner
participant PP as BasePopover.Popup
U->>TR: click / focus
TR->>PR: toggle open state
alt open
PR->>CT: render with props (placement | side/align)
note over CT: placement → side/align via mapping
CT->>PT: mount portal (optional containerRef)
PT->>PS: position with finalSide/finalAlign, sideOffset
PS->>PP: render popup content (children)
else close
PR-->>CT: unmount content
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (7)
packages/propel/src/popover/root.tsx (6)
60-89: Forward the ref through PopoverContent (Panel) to BasePopover.PopupConsumers won’t be able to attach a ref to Panel, and some focus-management/useImperativeHandle patterns will break. Wrap with forwardRef and pass it to BasePopover.Popup.
Apply this diff:
-// PopoverContent component -const PopoverContent = React.memo<PopoverContentProps>(function PopoverContent({ - children, - className, - placement, - side = "bottom", - align = "center", - sideOffset = 8, - containerRef, - ...props -}) { +// PopoverContent component +type PopupElement = React.ElementRef<typeof BasePopover.Popup>; +const PopoverContent = React.memo( + React.forwardRef<PopupElement, PopoverContentProps>(function PopoverContent( + { + children, + className, + placement, + side = "bottom", + align = "center", + sideOffset = 8, + containerRef, + ...props + }, + ref + ) { // side and align calculations const { finalSide, finalAlign } = React.useMemo(() => { if (placement) { const converted = convertPlacementToSideAndAlign(placement); return { finalSide: converted.side, finalAlign: converted.align }; } return { finalSide: side, finalAlign: align }; }, [placement, side, align]); return ( <PopoverPortal container={containerRef?.current}> <PopoverPositioner side={finalSide} sideOffset={sideOffset} align={finalAlign}> - <BasePopover.Popup data-slot="popover-content" className={className} {...props}> + <BasePopover.Popup ref={ref} data-slot="popover-content" className={className} {...props}> {children} </BasePopover.Popup> </PopoverPositioner> </PopoverPortal> ); -}); +}) +);
92-95: Consider forwarding refs on Trigger and Root for consistencyForwarding refs on Trigger and Root improves composability (e.g., focusing the trigger, integrating with form libs). Optional but recommended for DS components.
Apply this diff:
-const PopoverTrigger = React.memo<React.ComponentProps<typeof BasePopover.Trigger>>(function PopoverTrigger(props) { - return <BasePopover.Trigger data-slot="popover-trigger" {...props} />; -}); +const PopoverTrigger = React.memo( + React.forwardRef< + React.ElementRef<typeof BasePopover.Trigger>, + React.ComponentProps<typeof BasePopover.Trigger> + >(function PopoverTrigger(props, ref) { + return <BasePopover.Trigger ref={ref} data-slot="popover-trigger" {...props} />; + }) +); -const Popover = Object.assign( - React.memo<React.ComponentProps<typeof BasePopover.Root>>(function Popover(props) { - return <BasePopover.Root data-slot="popover" {...props} />; - }), +const Popover = Object.assign( + React.memo( + React.forwardRef< + React.ElementRef<typeof BasePopover.Root>, + React.ComponentProps<typeof BasePopover.Root> + >(function Popover(props, ref) { + return <BasePopover.Root ref={ref} data-slot="popover" {...props} />; + }) + ), { Button: PopoverTrigger, Panel: PopoverContent, } );Also applies to: 105-113
5-20: “auto/auto-start/auto-end” don’t behave as auto; they’re mapped to static bottom variantsRight now, specifying placement="auto" resolves to bottom/center, not an actual auto-position. This can be surprising to consumers.
Suggested options:
- Implement true auto/fallback behavior if BasePopover.Positioner supports it (e.g., via middleware/flip/autoPlacement).
- Or, remove auto/auto-start/auto-end from the public Placement union until supported.
- At minimum, document that auto* currently map to bottom variants.
If you choose to remove auto*, here’s a minimal type clean-up:
export type Placement = - | "auto" - | "auto-start" - | "auto-end" | "top-start" | "top-end" | "bottom-start" | "bottom-end" | "right-start" | "right-end" | "left-start" | "left-end" | "top" | "bottom" | "right" | "left"; @@ - ["auto", { side: "bottom", align: "center" }], - ["auto-start", { side: "bottom", align: "start" }], - ["auto-end", { side: "bottom", align: "end" }],I can help wire true auto placement if Positioner exposes the needed controls—confirm what’s available.
Also applies to: 34-50, 53-58
25-31: Prefer a direct container prop over containerRef for PortalPassing a ref object only to derive current is awkward for consumers. Consider supporting container?: HTMLElement | null, and deriving from containerRef for back-compat.
Example change:
export interface PopoverContentProps extends React.ComponentProps<typeof BasePopover.Popup> { placement?: Placement; align?: Align; sideOffset?: React.ComponentProps<typeof BasePopover.Positioner>["sideOffset"]; side?: Side; - containerRef?: React.RefObject<HTMLElement>; + container?: HTMLElement | null; + containerRef?: React.RefObject<HTMLElement>; // deprecated: prefer `container` } @@ - containerRef, + container, + containerRef, @@ - <PopoverPortal container={containerRef?.current}> + <PopoverPortal container={container ?? containerRef?.current}>If you’d like, I can follow up with a deprecation JSDoc on containerRef.
Also applies to: 80-88
72-79: useMemo here is unnecessary micro-optimizationThe conversion is trivial; memoizing adds complexity and doesn’t buy much. Optional simplification:
Possible simplification:
- Compute once without useMemo:
- const converted = placement ? convertPlacementToSideAndAlign(placement) : { side, align };
- const finalSide = converted.side; const finalAlign = converted.align;
122-122: Nit: spacing in named exportAlign with common formatting.
-export { Popover}; +export { Popover };packages/propel/src/popover/index.ts (1)
1-1: LGTM: Barrel re-export is straightforwardThis re-exports the public API cleanly. Optionally, you could switch to explicit named exports for clearer treeshaking boundaries, but not required.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
packages/propel/src/popover/index.ts(1 hunks)packages/propel/src/popover/root.tsx(1 hunks)
🔇 Additional comments (1)
packages/propel/src/popover/root.tsx (1)
116-121: Display names set — good for DevTools DXSetting displayName on the components/wrappers helps debugging.
3 participants
Description
This PR adds the Popover component to the Propel UI package.
Summary by CodeRabbit