Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork1.8k
Feature/tooltip closing#6007
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:main
Are you sure you want to change the base?
Feature/tooltip closing#6007
Uh oh!
There was an error while loading.Please reload this page.
Conversation
codecovbot commentedJun 25, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Bundle ReportChanges will increase total bundle size by 8.62kB (0.37%) ⬆️. This is within theconfigured threshold ✅ Detailed changes
Affected Assets, Files, and Routes:view changes for bundle: recharts/bundle-cjsAssets Changed:
|
codecovbot commentedJun 25, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@## main #6007 +/- ##==========================================- Coverage 96.26% 96.25% -0.02%========================================== Files 194 198 +4 Lines 19677 19887 +210 Branches 4039 4069 +30 ==========================================+ Hits 18942 19142 +200- Misses 729 739 +10 Partials 6 6 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This PR is going to need some work before we can look at landing it, it looks like there is a lot of unneeded and repeated code as well as tests. Maybe cursor got a little out of hand?
src/cartesian/Bar.tsx Outdated
| //@ts-expect-error BarRectangleItem type definition says it's missing properties, but I can see them present in debugger! | ||
| constonClick=onClickFromContext(entry,i); | ||
| // Always attach external event handlers (passed as props) | ||
| consteventHandlers:any={ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
can this be typed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This has been resolved on the commit35036e2
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
looks like this is still there, just in a different place
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
| externalOnClick?:(node:TreemapNode,e:React.MouseEvent<SVGPathElement,MouseEvent>)=>void; | ||
| externalOnMouseEnter?:(node:TreemapNode,e:React.MouseEvent<SVGPathElement,MouseEvent>)=>void; | ||
| externalOnMouseLeave?:(node:TreemapNode,e:React.MouseEvent<SVGPathElement,MouseEvent>)=>void; | ||
| internalOnClick?:(node:TreemapNode)=>void; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
why do we need these? There are already events here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Hi@ckifer these event handler props are necessary to make sure that the user-provided event handlers are actually invoked when the relevant DOM events occur on the rendered nodes, while still allowing the component to run its own internal logic for those events. I had to come up with this solution otherwise the test "navigates through nested nodes correctly" in line 220 of Treemap.spec.tsx fails because no click handler exist (internalOnClick) and it had no navigation logic (the Treemap doesn't change its view when clicked).
What would you advise me to do in this case? Keep the external event handlers to maintain the component's public API and navigation functionality or remove the navigation test? Perhaps I am missing something else. Let me know, thanks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
trying to understand if these are being exposed in the public interface or not. If so, we can't do this
src/state/tooltipSlice.ts Outdated
| /** | ||
| * This is a flag that is set when the tooltip is closing. | ||
| * This is used to prevent the tooltip from being closed again. | ||
| */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Can't we just check state and derive that the tooltip is not showing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
The flag prevents race conditions during the brief window when a tooltip is being closed but the middleware hasn't finished processing the close event. I have updated it the comment on a new commitc6d6244 . Please let me know if you consider that I should seek a different solution.
src/chart/RechartsWrapper.tsx Outdated
| consthandleRequestTooltipClose=useCallback(()=>{ | ||
| dispatch(tooltipCloseStart()); | ||
| setTimeout(()=>{ | ||
| dispatch(tooltipCloseEnd()); | ||
| },100); | ||
| },[dispatch]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I don't think we want to introduce timeouts if we can help it, this seems like something that might cause some funky bugs in the long run
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Thanks for the feedback, I completely agree about the bugs it could introduce in the long term.
I've updated the PR with a new approach that removes the timer entirely. Now, the mouseMove event immediately following a close action is caught by the middleware, which then "consumes" a gate flag and resets the state. This is now on this new commit5b5258a
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Pull Request Overview
Implements closing click-triggered tooltips when clicking outside the chart area.
- Introduces
isClosingflag and new Redux actions (tooltipCloseStart,tooltipCloseEnd,clearClickTooltip) - Adds outside‐click detection in
RechartsWrapperto dispatch tooltip‐close actions - Updates chart components (RadialBar, Pie, Scatter, Funnel, Bar) to merge external event handlers with internal tooltip logic and adds comprehensive tests
Reviewed Changes
Copilot reviewed 20 out of 20 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/state/tooltipSlice.ts | AddisClosing flag, new close actions and reducer handling |
| src/chart/RechartsWrapper.tsx | Detect outside clicks, dispatch close actions with delay, refactor clone |
| src/chart/{RadialBar, Pie, Scatter…} | Merge and forward external event handlers alongside tooltip handlers |
| test/component/Tooltip/* | New and updated tests for outside-click behavior and state expectations |
Comments suppressed due to low confidence (3)
src/component/Tooltip.tsx:125
- The new
onRequestTooltipCloseprop should be added to public docs/README so users know how to provide a custom close button or logic in custom tooltip content.
onRequestTooltipClose?: () => void;test/component/Tooltip/tooltipEventType.spec.tsx:237
- These debug assertions (expect(trigger).toBeDefined(), etc.) are not needed and clutter the test. Consider removing them once you confirm the variables are properly in scope.
expect(trigger).toBeDefined();test/component/Tooltip/tooltipEventType.spec.tsx:359
- This debug check isn't verifying production behavior and should be removed to keep tests focused on actual tooltip visibility.
expect(itemSelector).toBeDefined();Uh oh!
There was an error while loading.Please reload this page.
| document.addEventListener('mousedown',handleDocumentClick,true); | ||
| return()=>{ | ||
| document.removeEventListener('mousedown',handleDocumentClick,true); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Consider listening to 'pointerdown' or adding 'touchstart' in addition to 'mousedown' so the outside-click behavior works on touch devices without delay.
| document.addEventListener('mousedown',handleDocumentClick,true); | |
| return()=>{ | |
| document.removeEventListener('mousedown',handleDocumentClick,true); | |
| document.addEventListener('mousedown',handleDocumentClick,true); | |
| document.addEventListener('pointerdown',handleDocumentClick,true); | |
| return()=>{ | |
| document.removeEventListener('mousedown',handleDocumentClick,true); | |
| document.removeEventListener('pointerdown',handleDocumentClick,true); |
Copilot uses AI. Check for mistakes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Added 'pointerdown' on the latest commit
Simplify event handling in Bar, Funnel, Scatter, Pie, and RadialBar byremoving manual event handler combination. Context hooks already handleboth internal and external handlers.
Remove unnecessary React.cloneElement from RechartsWrapper
Extract repeated event handler pattern across chart components into a reusableutility function to improve maintainability and reduce code duplication.- Add createEventHandlers() utility in ReactUtils.ts- Define proper TypeScript types: TriggerInfo, ContextHandler, ContextHandlers- Refactor Bar, Funnel, Scatter, Pie, and RadialBar components- Fix TriggerInfo type definition for proper TooltipPayload handling- Remove explicit type annotations to avoid conflicts
isClosing type comment updated
fix: replace tooltip timeout with event-driven closingRemove arbitrary 100ms timeout and use event-driven approach for tooltipclosing. This eliminates race conditions and improves reliability byconsuming the isClosing gate in mouseEventsMiddleware instead of guessingtiming with setTimeout.
…pClose propRemove onRequestTooltipClose callback prop and simplify click-triggeredtooltip close handling by directly dispatching clearClickTooltip action.This eliminates the intermediate callback layer and makes the tooltipclose mechanism more direct and maintainable.
Will take a look at the changes tomorrow my time, thanks for making them! |
Remove the isClosing state and tooltipCloseStart/tooltipCloseEnd actions from the tooltip system. The tooltip closing functionality now exclusively uses the existing clearClickTooltip action, which is dispatched directly by the middleware for "tap away" scenarios and by the useEffect for "outside clicks" in RechartsWrapper.This simplifies the tooltip state management and removes unnecessary complexity while maintaining the same functionality for closing click-triggered tooltips.- Remove isClosing property from TooltipState type- Remove tooltipCloseStart and tooltipCloseEnd actions- Update test files to remove references to deleted state and actions
clean up Treemap tests
Add 'pointerdown' listener for better outside-click on touch devices
Hi@ckifer thanks for all your feedback and thorough check of the PR. I ran into some original errors while trying to implement this feature that led to the addition of unnecessary code and a bit of overuse of Cursor. I’ve now addressed most of the comments and cleaned up the repeated code and tests. There is one question for you on one of the comments. Please let me know about it and if there is anything else I should look into it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Looking better, thank you. I still want to pull this down and test it a bit. Added a few nits and questions
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
| if(isLeaf||type==='nest'){ | ||
| event={ | ||
| onMouseEnter:this.handleMouseEnter.bind(this,nodeProps), | ||
| onMouseLeave:this.handleMouseLeave.bind(this,nodeProps), | ||
| onClick:this.handleClick.bind(this,nodeProps), | ||
| }; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
doesn't this break nesting functionality?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Also why is this change here? How is it related to tooltip closing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
No on my end, is there anything in particular I should test to dig further?
| constcolors=colorPanel||COLOR_PANEL; | ||
| return( | ||
| <g> | ||
| <gonMouseEnter={onMouseEnter}onMouseLeave={onMouseLeave}onClick={onClick}> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
why move these to the group?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
If not, this test fails.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Please let me know if I should seek a different solution.
| externalOnClick?:(node:TreemapNode,e:React.MouseEvent<SVGPathElement,MouseEvent>)=>void; | ||
| externalOnMouseEnter?:(node:TreemapNode,e:React.MouseEvent<SVGPathElement,MouseEvent>)=>void; | ||
| externalOnMouseLeave?:(node:TreemapNode,e:React.MouseEvent<SVGPathElement,MouseEvent>)=>void; | ||
| internalOnClick?:(node:TreemapNode)=>void; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
trying to understand if these are being exposed in the public interface or not. If so, we can't do this
src/polar/Pie.tsx Outdated
| onClick:props.onClick, | ||
| onMouseEnter:props.onMouseEnter, | ||
| onMouseLeave:props.onMouseLeave, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
do these need included here? I think they just get filtered out below?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
You're right, those lines are redundant. The event handlers are passed down through the rest of the props and picked up by createEventHandlers. I'd removed them on the commitbbcc682
| contextHandlers:ContextHandlers, | ||
| ):any=>{ | ||
| // Always attach external event handlers (passed as props) | ||
| consteventHandlers:any={ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Can this be typed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Also can this be in separate PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Hi@ckifer and@PavelVanecek I really tried to typed properly but I couldn't as the Bar.tsx expects one type and the rest another, and both won't accept to have both types on it. Happy to create another PR, question about how should I integrate this with my current PR as well?
| ), | ||
| mouseHoverSelector:funnelChartMouseHoverTooltipSelector, | ||
| expectedTransform:'transform: translate(355px, 55px);', | ||
| expectedTransform:'transform: translate(40px, 15px);', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
why did this change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This looks like a bug to me. Why did this coordinate change? I do not see how it's related to the PR target.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I had reverted back he changes and updated the trigerInfo function in the Funnel.tsx and solved the issue. The tooltip was being positioned at the top-left corner of the funnel segment instead of its center. I've corrected the logic to use the proper tooltipPosition, and the test now passes with the original expected value. See this commit
1969150 about it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Little bit larger than I would like but okay. Can you please figure out the failing test, and mocks, before merging?
| if(isLeaf||type==='nest'){ | ||
| event={ | ||
| onMouseEnter:this.handleMouseEnter.bind(this,nodeProps), | ||
| onMouseLeave:this.handleMouseLeave.bind(this,nodeProps), | ||
| onClick:this.handleClick.bind(this,nodeProps), | ||
| }; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Also why is this change here? How is it related to tooltip closing?
| activeCoordinate:activeProps.activeCoordinate, | ||
| }), | ||
| ); | ||
| }else{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This looks like the only relevant change to the PR title.
| contextHandlers:ContextHandlers, | ||
| ):any=>{ | ||
| // Always attach external event handlers (passed as props) | ||
| consteventHandlers:any={ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Also can this be in separate PR?
| ), | ||
| mouseHoverSelector:funnelChartMouseHoverTooltipSelector, | ||
| expectedTransform:'transform: translate(355px, 55px);', | ||
| expectedTransform:'transform: translate(40px, 15px);', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This looks like a bug to me. Why did this coordinate change? I do not see how it's related to the PR target.
| store=createMinimalStore(); | ||
| // Mock the selector to return valid active props | ||
| constmockSelectActivePropsFromChartPointer=vi.mocked( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Why so many mocks? Can we replace the mocks with actual interactions? That way we will not depend on the selector/state implementation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I have simplified and updated the test file with actual interactions on this commitfd439be
Move noop function definition to createEventHandlers utility and remove redundant noop imports from individual components.
Commen removed as per request
return null back as it was
Hi@ckifer this is in response tothis comment that I cannot reply below it. As per my understanding of the code the externalOnClick, externalOnMouseEnter, externalOnMouseLeave, and internalOnClick props are not part of the public API. They are internal props within the ContentItemWithEvents component. |
Remove redundant event handler props
The tooltip for the FunnelChart was being positioned at the top-left corner of the trapezoid segment instead of its center.This commit updates the event handler logic to use the correctly calculated `entry.tooltipPosition`, which ensures the tooltip appears centered on the active funnel segment. This resolves the failing Tooltip visibility test and aligns the component's behavior with the expected output.
The tests cover two key scenarios:- Clearing an active click-tooltip when the user clicks on an empty chart area.- Ensuring the tooltip state remains unchanged if no tooltip is active during the click.This validates the "tap-away" to dismiss functionality.
Implement tooltip close on outside click for trigger="click"
Description
This PR implements the ability to close click-triggered tooltips by clicking outside the chart area. Previously, tooltips with trigger="click" would remain open until the user clicked on another chart element or manually closed them. Now, clicking anywhere outside the chart will close the tooltip.
Related Issue
#3573
Motivation and Context
Users expect click-triggered tooltips to behave like standard UI tooltips that close when clicking outside their area. This is a common UX pattern that improves usability and provides a more intuitive interaction model for charts with click-triggered tooltips.
How Has This Been Tested?
Test Environment:
Types of changes
Checklist:
Note: The implementation includes: