Conversation
…proved performance and structure
Hello jeanmarcmilletscality,My role is to assist you with the merge of this Available options
Available commands
Status report is not available. |
Waiting for approvalThe following approvals are needed before I can proceed with the merge:
Peer approvals must include at least 1 approval from the following list: |
There was a problem hiding this comment.
Pull request overview
This PR refactors the Tooltip component to use React portals, preventing tooltip content from being clipped or affecting container dimensions. The tooltip overlay is now rendered directly to document.body instead of being nested in the DOM hierarchy.
- Refactored Tooltip component to render overlay using
createPortaltodocument.body - Updated story and preview configurations to demonstrate the fix with constrained container dimensions
- Changed overflow behavior from
scrolltoautofor better UX
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| src/lib/components/tooltip/Tooltip.component.tsx | Refactored tooltip to use createPortal, moving overlay rendering outside DOM hierarchy; simplified useEffect dependencies |
| stories/tablev2.stories.tsx | Added constrained dimensions to TableWithSyncButton story to demonstrate tooltip behavior in limited space |
| stories/common.tsx | Changed overflow from scroll to auto to prevent unnecessary scrollbars |
| .storybook/preview.js | Removed overflow: scroll from story wrapper to prevent scrollbar issues |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| }); | ||
| } | ||
| }, [tooltipRef.current, childrenRef.current, isTooltipVisible]); | ||
| }, [isTooltipVisible, placement]); |
There was a problem hiding this comment.
The useEffect dependencies array is missing childrenRef and tooltipRef. The effect uses childrenRef.current and tooltipRef.current internally. While refs themselves don't change, this could lead to stale closures. Consider adding these refs to the dependency array or using a callback ref pattern to ensure proper updates when the refs are set.
| }, [isTooltipVisible, placement]); | |
| }, [isTooltipVisible, placement, childrenRef, tooltipRef]); |
| onPointerEnter={() => { | ||
| setIsTooltipVisible(true); | ||
| }} | ||
| onPointerLeave={() => { | ||
| setIsTooltipVisible(false); | ||
| }} |
There was a problem hiding this comment.
[nitpick] The event handlers create new function instances on every render. Consider using useCallback to memoize these handlers for better performance, especially if the tooltip is used frequently in lists or tables.
Waiting for approvalThe following approvals are needed before I can proceed with the merge:
Peer approvals must include at least 1 approval from the following list: |
|
/approve |
|
I have successfully merged the changeset of this pull request
Please check the status of the associated issue None. Goodbye jeanmarcmilletscality. The following options are set: approve |
3 participants
Use createPortal to avoid issue with Tooltip inside container: Tooltip clipped or pushing container width or height