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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[WIP] feat: click trigger treemap tooltip #4245
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## 3.x #4245 +/- ##
==========================================
- Coverage 93.21% 93.09% -0.13%
==========================================
Files 94 94
Lines 20028 20059 +31
Branches 2720 2721 +1
==========================================
+ Hits 18670 18674 +4
- Misses 1347 1374 +27
Partials 11 11 ☔ View full report in Codecov by Sentry. |
Hahaaa I like the bot harassing on new lines without coverage |
LOL, bad me for not having enough time for tests at midnight my time 🤣 |
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 wonder if I can convince you to move the tooltip behaviour to context while working on the Treemap onClick fix.
@@ -360,8 +360,9 @@ export class Treemap extends PureComponent<Props, State> { | |||
e.persist(); | |||
const { onMouseEnter, children } = this.props; | |||
const tooltipItem = findChildByType(children, Tooltip); | |||
const tooltipTrigger = tooltipItem.props.trigger; |
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.
Another findChildByType
😢 we need to move all of this to context
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.
Outside of this PR of course.
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.
yep. I'm solving problems from issues in a siloed way.. need to start helping with the broader changes.
const { activeNode } = this.state; | ||
const isNestedParentNode = type === 'nest' && node.children; | ||
|
||
if (tooltipTrigger === 'click') { |
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.
How much of this is copy-paste from generateCategoricalChart
? Just wondering.
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 just hacked this to get it it to work, no references from anywhere except Treemap (its not pretty I know :p)
I'll take a step back from this change and try to narrow in on moving the Tooltip to context instead. This functionality was worth a mini-POC either way. Definitely something to come back to |
Description
Realized that Treemap currently has no on-click Tooltip ability. Hacked this together quickly, adding as draft for now. Watch video for behavior
TODO: TESTS!!
Related Issue
#4238
Motivation and Context
Feature parity with other charts
How Has This Been Tested?
Screenshots (if appropriate):
recharts-click-tooltip.mov
Types of changes
Checklist: