[EuiToolTip] Refactor to a functional component, memoize style and improve tests#9570
Conversation
There was a problem hiding this comment.
This is an expected change. Previously, position leaked to the DOM element but it's not a valid HTML attribute. Same as offset.
d9d22cc to
64f6572
Compare
| const setPopoverRef = useCallback( | ||
| (el: HTMLElement) => { | ||
| popoverRef.current = el; | ||
| if (el) positionToolTip(); |
There was a problem hiding this comment.
In commit: a38e23d
This line is the only change from the diff. Otherwise, we are just moving positionToolTip to be before the setPopoverRef that calls it.
if (el) positionToolTip(); fixes tooltip not showing in StrictMode because EuiResizeObserver (class component) has ResizeObserver disconnected by StrictMode's simulated unmount before the initial callback fires, so positionToolTip is never called. Calling it directly from setPopoverRef every mount ensures that it gets positioned correctly.
This doesn't affect anything detrimentally.
19449a9 to
a38e23d
Compare
| }; | ||
| const setPopoverRef = useCallback( | ||
| (ref: HTMLDivElement) => { | ||
| popover.current = ref; |
There was a problem hiding this comment.
Adding this line fixes pre-existing bug where the tooltip wouldn't reposition on resizing the window.
There was a problem hiding this comment.
Pull request overview
Refactors EuiToolTip from a class component to a functional component with hooks, updates related components/usages, and rewrites/expands tests to use more meaningful assertions while also addressing tooltip repositioning behavior.
Changes:
- Migrate
EuiToolTiptoforwardRef+ hooks and expose an imperative ref API (EuiToolTipRef). - Memoize tooltip/anchor styles via
useEuiMemoizedStylesand adjust tooltip popover behavior. - Rewrite and expand Jest tests + update snapshots across tooltip consumers.
Reviewed changes
Copilot reviewed 16 out of 25 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/eui/src/components/tool_tip/tool_tip.tsx | Converts EuiToolTip to a hook-based functional component and introduces EuiToolTipRef. |
| packages/eui/src/components/tool_tip/tool_tip.test.tsx | Rewrites tooltip unit tests to rely less on snapshots and add behavioral assertions. |
| packages/eui/src/components/tool_tip/tool_tip.stories.tsx | Adds new QA-focused stories (currently flagged as TODO to remove). |
| packages/eui/src/components/tool_tip/tool_tip_popover.tsx | Switches to memoized styles and adds resize-driven repositioning logic. |
| packages/eui/src/components/tool_tip/tool_tip_manager.test.ts | Improves test isolation and expands assertions around singleton behavior. |
| packages/eui/src/components/tool_tip/tool_tip_anchor.tsx | Memoizes anchor styles via useEuiMemoizedStyles. |
| packages/eui/src/components/tool_tip/index.ts | Re-exports EuiToolTipRef type from the public tooltip entrypoint. |
| packages/eui/src/components/tool_tip/icon_tip.test.tsx | Updates IconTip tests away from prop-specific snapshots toward behavioral checks. |
| packages/eui/src/components/tool_tip/snapshots/tool_tip.test.tsx.snap | Updates snapshots for tooltip rendering changes. |
| packages/eui/src/components/tool_tip/snapshots/icon_tip.test.tsx.snap | Removes outdated snapshots tied to previous IconTip prop rendering expectations. |
| packages/eui/src/components/selectable/selectable_list/selectable_list_item.tsx | Updates tooltip ref usage to the new EuiToolTipRef API. |
| packages/eui/src/components/selectable/selectable_list/snapshots/selectable_list_item.test.tsx.snap | Updates snapshots for tooltip attribute changes in selectable list items. |
| packages/eui/src/components/filter_group/filter_select_item.tsx | Updates tooltip ref typing to EuiToolTipRef. |
| packages/eui/src/components/filter_group/filter_select_item.test.tsx | Adds/updates tests for programmatic tooltip show/hide behavior. |
| packages/eui/src/components/context_menu/snapshots/context_menu_item.test.tsx.snap | Updates tooltip snapshot output (removes invalid DOM attrs). |
| packages/eui/src/components/collapsible_nav_beta/collapsible_nav_item/collapsed/snapshots/collapsed_nav_button.test.tsx.snap | Updates tooltip snapshot output (removes invalid DOM attrs). |
| packages/eui/.loki/reference/chrome_mobile_Forms_EuiFilePicker_Controlled_With_Files.png | Adds/updates Loki reference image (appears unrelated to tooltip refactor). |
| packages/eui/.loki/reference/chrome_desktop_Forms_EuiForm_EuiFormControlLayoutDelimited_Kitchen_Sink.png | Adds/updates Loki reference image (appears unrelated to tooltip refactor). |
| packages/eui/.loki/reference/chrome_desktop_Forms_EuiForm_EuiFormControlLayoutDelimited_High_Contrast.png | Adds/updates Loki reference image (appears unrelated to tooltip refactor). |
| packages/eui/.loki/reference/chrome_desktop_Forms_EuiFilePicker_Controlled_With_Files.png | Adds/updates Loki reference image (appears unrelated to tooltip refactor). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| display?: (typeof DISPLAYS)[number]; | ||
| /** | ||
| * Delay before showing tooltip. Good for repeatable items. | ||
| */ | ||
| delay: ToolTipDelay; | ||
| delay?: ToolTipDelay; | ||
| /** | ||
| * An optional title for your tooltip. | ||
| */ | ||
| title?: ReactNode; | ||
| /** | ||
| * Unless you provide one, this will be randomly generated. | ||
| */ | ||
| id?: string; | ||
| /** | ||
| * Suggested position. If there is not enough room for it this will be changed. | ||
| */ | ||
| position: ToolTipPositions; | ||
| position?: ToolTipPositions; |
There was a problem hiding this comment.
PR description states "No API changes", but this diff makes delay optional in the public EuiToolTipProps type. Even though it's non-breaking, it's still an exported type surface change and should be reflected in the PR's API section (or reverted if unintended).
There was a problem hiding this comment.
EuiToolTip can absolutely be used without providing delay and position. position is set to top and delay to regular by default.
So no, this is not an API change. delay and position are in fact typed as optional.
| export type { ToolTipPositions } from './tool_tip_popover'; | ||
| export type { EuiToolTipProps } from './tool_tip'; | ||
| export type { EuiToolTipProps, EuiToolTipRef } from './tool_tip'; | ||
| export { EuiToolTip } from './tool_tip'; |
There was a problem hiding this comment.
PR description states "No API changes", but this diff re-exports a new public type (EuiToolTipRef) from the tool_tip index. Even if it's additive, please update the PR's API section to reflect the exported surface change (or keep it internal if not intended as public API).
There was a problem hiding this comment.
It is an undocumented public API. Let's leave this be.
tkajtoch
left a comment
There was a problem hiding this comment.
Code changes look great! I'm proceeding to manual testing
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 16 out of 25 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (1)
packages/eui/src/components/tool_tip/tool_tip.tsx:396
- When neither
contentnortitleare provided,showToolTipcan still setvisible=true, which causes the trigger to receivearia-describedby(viaisVisible) even though no tooltip element is rendered (visible && (content || title)gate). This produces anaria-describedbythat points to a non-existent element. Consider short-circuitingshowToolTip/onFocus/onMouseOver(and/or theid/isVisiblewiring) when there is no tooltip content so thatvisiblenever becomes true in this case.
return (
<>
<EuiToolTipAnchor
{...anchorProps}
ref={setAnchorRef}
onBlur={onBlur}
onFocus={onFocus}
onKeyDown={onEscapeKey}
onMouseOver={showToolTip}
onMouseOut={onMouseOut}
// `id` defines if the trigger and tooltip are automatically linked via `aria-describedby`.
id={!disableScreenReaderOutput ? id : undefined}
className={anchorClasses}
display={display}
isVisible={visible}
>
{children}
</EuiToolTipAnchor>
{visible && (content || title) && (
<EuiPortal>
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
tkajtoch
left a comment
There was a problem hiding this comment.
Code changes look good. I went through all QA steps and tested some more combinations - everything works as expected.
While testing StrictMode, I've noticed that the tooltip doesn't appear on initial focus via autoFocus, only on subsequent hover/focus events.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 16 out of 25 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
I tested the StrictMode autoFocus fix locally and can confirm it works as expected. Thank you for addressing it! |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 15 out of 24 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
💚 Build SucceededHistory
|
💚 Build Succeeded
History
|
Tip
There's not a lot of changes but I do recommend following commits.
Warning
Revert 68bbc21 before merging!
Summary
EuiToolTipto a functional component, memoized its styles, updated the usages and improved the test suite. Additionally, fixed a bug where the tooltip doesn't reposition when resizing the window.positionanddelaymarked as required when they aren't #9593repositionOnScroll, persist on click behavior, delay etc. There's supposed to be NO REGRESSION.API Changes
🟢 No API changes.
Screenshots
🟢 No visual changes.
Impact Assessment
Note: Most PRs should be tested in Kibana to help gauge their Impact before merging.
Impact level: 🟢 Low
(WIP - will update snapshots for Kibana before merging)
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
EuiToolTipStory: Playground
Escape→ tooltip hidespositionprop → tooltip appears on correct side (top/right/bottom/left)delaytolong→ tooltip appears after ~1250msdisplaytoblock→ anchor wrapper becomes block-leveltitleprop → title appears above content with a dividerdisableScreenReaderOutput=true→aria-describedbyis NOT added to the trigger (note:aria-describedbyis added dynamically when tooltip is shown)offset→ tooltip moves closer/further from anchor (in all directions, changeposition)repositionOnScroll=trueand scroll the page → tooltip follows anchor (e.g. set the container to have a largeheightin DevTools)Pre-existing issue:
EuiIconTipStory: Playground
aria-labelis read by screen reader (setaria-labelprop and use VoiceOver/NVDA)type,color,sizeprops affect the iconEuiFilterSelectItemStory: FilterGroup / Multiple Popovers
showToolTip())hideToolTip())EuiSelectableListItemStory: Selectable / With Tooltip
Use Arrow keys to navigate within the selectable list.
showToolTip())hideToolTip())aria-describedbyon the.euiSelectableListItem__contentis set to the tooltip'sid(check with browser inspector)EuiCollapsedNavButtonStory: CollapsibleNav (Beta) / Playground
EuiContextMenuItemStory: ContextMenu / Playground
toolTipContentshows tooltip on hoverEscape key propagation
Inside a flyout
Escapecloses tooltip only, does not close the flyoutEscapecloses the flyoutInside a modal
Escapecloses tooltip only, does not close the modalEscapecloses the modalInside a popover
Escapecloses tooltip only, does not close the popoverEscapecloses the popoverStrict Mode
<StrictMode>renders correctlyChecklist before marking Ready for Review
QA: Tested docs changesChangelog: Added changelog entryBreaking changes: Addedbreaking changelabel (if applicable)VRT passed:
There were unrelated image reference updates. I added them on 451f7a5
Reviewer checklist