fix(pds-popover): convert to slot for trigger and portal rendering #574
Conversation
- Move popover content to document.body using portal approach to escape CSS stacking contexts - Add scroll and resize listeners with debouncing for dynamic positioning updates - Implement proper focus management with Pine design system focus ring styles - Move actual DOM nodes instead of cloning to preserve event handlers and component state - Add timestamp-based light dismiss to prevent immediate closure on open - Add guards for repositioning and zero-dimension edge cases - Ensure proper cleanup of event listeners and portal elements on disconnect - Document unused shadow DOM styles (replaced by inline portal styles)
✅ Deploy Preview for pine-design-system ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
github-actions
bot
added
bug
pixelflips
changed the title
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
📝 WalkthroughWalkthroughRefactors pds-popover from an inline, text-prop-driven component to a portal-based popover with slot-driven triggers. Removes the Pre-merge checks and finishing touches✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![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
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
libs/core/src/components.d.ts(0 hunks)libs/core/src/components/pds-popover/docs/pds-popover.mdx(6 hunks)libs/core/src/components/pds-popover/pds-popover.scss(1 hunks)libs/core/src/components/pds-popover/pds-popover.tsx(4 hunks)libs/core/src/components/pds-popover/popover-interface.ts(0 hunks)libs/core/src/components/pds-popover/readme.md(1 hunks)libs/core/src/components/pds-popover/stories/pds-popover.stories.js(2 hunks)libs/core/src/components/pds-popover/test/pds-popover.e2e.ts(1 hunks)libs/core/src/components/pds-popover/test/pds-popover.spec.tsx(2 hunks)
💤 Files with no reviewable changes (2)
- libs/core/src/components/pds-popover/popover-interface.ts
- libs/core/src/components.d.ts
|
@coderabbitai addressed issue. please review again |
|
@pixelflips: I'll review the changes now. ✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
libs/core/src/components/pds-popover/pds-popover.tsx (1)
620-620: Add missing semicolon for consistency.The
offsetconstant declaration is missing a trailing semicolon, which is inconsistent with the rest of the codebase.- const offset = 8 + const offset = 8;
🧹 Nitpick comments (1)
libs/core/src/components/pds-popover/stories/pds-popover.stories.js (1)
68-76: Consider a more defensive close handler.The inline onclick handler assumes the element exists and is a pds-popover. While this works for the story, a more robust approach would prevent potential errors if the pattern is copied elsewhere.
Consider this alternative:
<pds-button variant="unstyled" icon-only - onclick="document.getElementById('${args.componentId || 'popover-rich'}').hide()" + onclick="this.closest('pds-popover')?.hide()" aria-label="Close" >This uses
closest()to find the parent popover, eliminating the need for ID matching and making the code more portable.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
libs/core/src/components/pds-popover/pds-popover.tsx(4 hunks)libs/core/src/components/pds-popover/stories/pds-popover.stories.js(2 hunks)libs/core/src/components/pds-popover/test/pds-popover.spec.tsx(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: test-specs (20)
- GitHub Check: test-specs (18)
- GitHub Check: test-specs (22)
🔇 Additional comments (4)
libs/core/src/components/pds-popover/pds-popover.tsx (3)
114-221: Excellent lifecycle management.The component properly:
- Binds event handlers once for efficient cleanup
- Creates the portal on mount with a fallback trigger initialization
- Cleans up all listeners, timers, and DOM elements on disconnect
This prevents memory leaks and ensures reliable teardown.
308-356: Well-designed portal content synchronization.The implementation correctly:
- Preserves node identity, handlers, and state when moving to portal
- Uses placeholder comments to track original positions
- Prevents duplicate moves with isAlreadyInPortal and isAlreadyTracked checks
- Restores content to original positions on hide
Combined with the content slot change handler (line 690), this ensures the portal stays in sync with dynamic slot updates while the popover is visible.
509-559: Robust light dismiss implementation.The light dismiss logic properly:
- Prevents the opening click from immediately closing via timestamp comparison
- Handles the DOMHighResTimeStamp to epoch conversion correctly (well-commented)
- Stops Escape key propagation to exclusively handle popover closure
- Returns focus to trigger on Escape, matching native Popover API behavior
libs/core/src/components/pds-popover/test/pds-popover.spec.tsx (1)
1-825: Excellent comprehensive test coverage.The test suite thoroughly covers:
- Portal lifecycle (creation, removal, singleton behavior)
- All 12 positioning placements with mocked geometry
- Trigger and content slot handling including dynamic changes
- Public API methods (show, hide, toggle)
- Light dismiss behavior with timing considerations
- Edge cases (null elements, unmounted state, repositioning guards, zero-dimension triggers)
- Event emissions and ARIA attribute updates
This level of coverage ensures the portal-based refactor works correctly across all scenarios.
Description
Refactors
pds-popoverto use a slot-based approach for the trigger element, improving flexibility and developer experience. The trigger is now passed via aslot="trigger"instead of usingpopover-target, allowing any element (buttons, links, custom components) to activate the popover.Key changes:
<slot name="trigger">for better flexibilitydocument.bodyfor correct positioning in all CSS contextsMotivation: The slot-based approach provides more flexibility for consumers and aligns with modern web component patterns. The portal implementation ensures the component works correctly in all usage contexts, regardless of parent container styles.
Fixes https://kajabi.atlassian.net/browse/DSS-1567
Type of change
How Has This Been Tested?
Test Configuration:
Checklist:
If not applicable, leave options unchecked.