Split Tooltip into TooltipBoundingBox #3951
Conversation
| @@ -33,30 +37,25 @@ function renderContent<TValue extends ValueType, TName extends NameType>( | |||
| return <DefaultTooltipContent {...props} />; | |||
| } | |||
|
|
|||
| export type TooltipProps<TValue extends ValueType, TName extends NameType> = DefaultProps<TValue, TName> & { | |||
| export type TooltipProps<TValue extends ValueType, TName extends NameType> = ToltipContentProps<TValue, TName> & { | |||
There was a problem hiding this comment.
Changes to props:
- "Sort lines alphabetically"
viewBoxuses (identical) imported type instead of inline.
| reverseDirection: { x: false, y: false }, | ||
| offset: 10, | ||
| viewBox: { x: 0, y: 0, height: 0, width: 0 }, | ||
| animationDuration: 400, |
There was a problem hiding this comment.
"Sort lines alphabetically"
| Math.abs(box.width - this.lastBoundingBox.width) > EPS || | ||
| Math.abs(box.height - this.lastBoundingBox.height) > EPS | ||
| ) { | ||
| this.lastBoundingBox.width = box.width; |
There was a problem hiding this comment.
I think this instance assignment is a potential bug but I copy pasted it without a change.
There was a problem hiding this comment.
If you think this is a bug, please do elaborate, and create a follow up issue. We can tackle this independently. I appreciate this you only copied it in this PR.
There was a problem hiding this comment.
I think @HHongSeungWoo recently modified this section?
There was a problem hiding this comment.
@HHongSeungWoo confirmed in another thread that this is intentional: #3852 (comment)
I do not have any evidence that this is a bug. It's only an antipattern that can lead to bugs - this thing here is why useState and useRef exists.
There was a problem hiding this comment.
(Reading defaultProps and cloning elements are also antipatterns 😬 )
There was a problem hiding this comment.
This should probably be a function component then no? 😅
There was a problem hiding this comment.
While we have many anti-patterns, I think this one is similar to useMemo.
Due to getTooltipTranslate, this seems to be the best option for now.
|
This will also make my PoC branches easier - there's tons of conflicts. |
src/component/TooltipBoundingBox.tsx
Outdated
| dismissedAtCoordinate: Coordinate; | ||
| }; | ||
|
|
||
| const EPS = 1; |
There was a problem hiding this comment.
Nit: If you know what EPS stands for, I would sure love to replace this with a proper name.
There was a problem hiding this comment.
"Epsilon"
denotes the smallest quantity like a term which is taken as the zero in some limit
There was a problem hiding this comment.
This is no longer necessary. It seems to have been added due to the structure of obtaining the tooltip's viewbox, which could have caused an infinite loop
## Description I think this is an opportunity to have two clearly defined components with split responsibility. One doing the position (BoundingBox) and one doing the content (Tooltip). ## Related Issue ## Motivation and Context ## How Has This Been Tested? unit tests ## Screenshots (if appropriate): ## Types of changes - [ ] Bug fix (non-breaking change which fixes an issue) - [ ] New feature (non-breaking change which adds functionality) - [ ] Breaking change (fix or feature that would cause existing functionality to change) ## Checklist: - [X] My code follows the code style of this project. - [ ] My change requires a change to the documentation. - [ ] I have updated the documentation accordingly. - [X] I have added tests to cover my changes. - [ ] I have added a storybook story or extended an existing story to show my changes - [X] All new and existing tests passed.
Description
I think this is an opportunity to have two clearly defined components with split responsibility. One doing the position (BoundingBox) and one doing the content (Tooltip).
Related Issue
Motivation and Context
How Has This Been Tested?
unit tests
Screenshots (if appropriate):
Types of changes
Checklist: