[EuiWrappingPopover] Migrate from class component to functional component #9471#9537
[EuiWrappingPopover] Migrate from class component to functional component #9471#9537parbhat-cpp wants to merge 3 commits intoelastic:mainfrom
Conversation
…cional-component Migrate/euiwrappingpopover funcional component
|
💚 CLA has been signed |
|
👋 Since this is a community submitted pull request, a Buildkite build has not been started automatically. Would an Elastic organization member please verify the contents of this pull request and kick off a build manually? |
github-actions
bot
added
the
community contribution
There was a problem hiding this comment.
Pull request overview
Refactors EuiWrappingPopover from a class component to a functional component while preserving its core behavior of portaling the popover next to an existing DOM anchor element and restoring that anchor on unmount.
Changes:
- Migrated
EuiWrappingPopoverfromComponentlifecycle methods to hooks (useLayoutEffect, refs, callbacks). - Replaced instance fields/methods with
useRef+useCallbackequivalents for portal/anchor handling.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const setAnchorRef = useCallback( | ||
| (node: HTMLDivElement | null) => { | ||
| node?.insertAdjacentElement('beforebegin', button); | ||
| }, | ||
| [button] | ||
| ); | ||
|
|
||
| useLayoutEffect(() => { | ||
| return () => { | ||
| if (button.parentNode && portalRef.current) { | ||
| portalRef.current.insertAdjacentElement('beforebegin', button); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| setPortalRef = (node: HTMLElement | null) => { | ||
| this.portal = node; | ||
| }; | ||
|
|
||
| setAnchorRef = (node: HTMLElement | null) => { | ||
| node?.insertAdjacentElement('beforebegin', this.props.button); | ||
| }; | ||
|
|
||
| render() { | ||
| const { button, ...rest } = this.props; | ||
|
|
||
| return ( | ||
| <EuiPortal | ||
| portalRef={this.setPortalRef} | ||
| insert={{ sibling: this.props.button, position: 'after' }} | ||
| > | ||
| <EuiPopover | ||
| {...rest} | ||
| button={ | ||
| <div | ||
| ref={this.setAnchorRef} | ||
| className="euiWrappingPopover__anchor" | ||
| /> | ||
| } | ||
| /> | ||
| </EuiPortal> | ||
| ); | ||
| } | ||
| }; | ||
| }, [button]); |
There was a problem hiding this comment.
EuiWrappingPopover's core behavior (moving the external button element into the portal/anchor and restoring it on unmount) changed implementation significantly but currently has no dedicated unit test coverage. Please add a test (e.g. in the popover test suite) that asserts: 1) after mount, the original button is moved into the popover anchor, and 2) after unmount, the button is restored to its original DOM position (not removed with the portal).
I have signed the agreement, thank you. |
Summary
EuiWrappingPopoverfrom class to functional Component.yarn test-unit,yarn test-unit popoverand through storybook.API Changes
noneScreenshots
Impact Assessment
- [ ] 🔴 Breaking changes — What will break? How many usages in Kibana/Cloud UI are impacted?- [ ] 💅 Visual changes — May impact style overrides; could require visual testing. Explain and estimate impact.- [ ] 🧪 Test impact — May break functional or snapshot tests (e.g., HTML structure, class names, default values).- [ ] 🔧 Hard to integrate — If integration is complex, stage commits in Kibana/Cloud UI branches for cherry-picking and link to them below.Impact level: 🟢 None
Release Readiness
- [ ] Documentation: {link to docs page(s)}- [ ] Figma: {link to Figma or issue}- [ ] Migration guide: {steps or link, for breaking/visual changes or deprecations}- [ ] Adoption plan (new features): {link to issue/doc or outline who will integrate this and where}QA instructions for reviewer
EuiWrappingPopoverto check if the component is working as expected.Checklist before marking Ready for Review
- [ ] QA: Tested in CodeSandbox and Kibana- [ ] QA: Tested docs changes- [ ] Tests: Added/updated Jest, Cypress, and VRT- [ ] Changelog: Added changelog entry- [ ] Breaking changes: Addedbreaking changelabel (if applicable)Reviewer checklist