new(vx-tooltip): add Portal + demo #755
Conversation
…l listeners to useMeasure config
|
|
||
| import Tooltip, { TooltipProps, defaultStyles } from './Tooltip'; | ||
|
|
||
| type RectShape = { |
There was a problem hiding this comment.
these were written before the @vx/bounds types were added
| @@ -0,0 +1,244 @@ | |||
| import React, { useState, useCallback } from 'react'; | |||
| import useMeasure from 'react-use-measure'; | |||
| import { Tooltip, TooltipWithBounds, useTooltip, defaultStyles, Portal } from '@vx/tooltip/src'; | |||
There was a problem hiding this comment.
reminder to update src => lib
There was a problem hiding this comment.
this is fixed in #756 which set to merge into this branch
|
|
||
| componentWillUnmount() { | ||
| if (this.node && document.body) { | ||
| document.body.removeChild(this.node); |
There was a problem hiding this comment.
can also do this.node.remove()
| /> | ||
| <TooltipWrapper> | ||
| <TooltipComponent | ||
| key={Math.random()} // needed for bounds to update correctly |
There was a problem hiding this comment.
Keys should be stable, predictable, and unique. Unstable keys (like those produced by Math.random()) will cause many component instances and DOM nodes to be unnecessarily recreated, which can cause performance degradation and lost state in child components.
https://reactjs.org/docs/reconciliation.html#trade-offs
Does it have to be recreated all the time?
There was a problem hiding this comment.
yeah I think unfortunately for now, we do this in most of our demos. This forces a re-mount (😱) to force-update the bounds the component has, which basically change on every mouse move. Without this, if you start the tooltip on the left of the chart and move it to the right edge, it still thinks it has room because it's using the initial bounds.
This really can't be that great for perf because of the remount and because calling .getBoundingClientRect() forces layout/reflow. I tried to switch to use-measure for the boundary detection, but it only uses scroll listeners + ResizeObserver, so it doesn't re-compute on mouse move.
There was a problem hiding this comment.
left a comment on react-use-measure about it cc @techniq
There was a problem hiding this comment.
IntersectionObserver seems promising. If react-use-measure doesn't add something, we might be able to only update the key when the observer fires... to force an update.
I initially thought we could pre-compute the bounds based on the parent and tooltip size, but sinse the tooltip size can change size based on the contents, this wouldn't work.
There was a problem hiding this comment.
I initially thought we could pre-compute the bounds based on the parent and tooltip size, but since the tooltip size can change size based on the contents, this wouldn't work.
ah this is interesting, currently we use the bounds.left/top attributes of the tooltip, rather than bounds.width/height (which should be more stable). I took a shot at updating to use the latter and not updating key, but hit issues in the portal where width/height of the parent can be 0 😭
new(vx-tooltip): add useTooltipInPortal
|
gonna merge this to wrap it up, happy to address any concerns in a followup PR |
🚀 Enhancements
📝 Documentation
This PR
Portalcomponent to@vx/tooltipuseTooltipleft: 0by default onTooltipWithBoundswhich was needed for correct positioning within aPortal/tooltip, which demos the boundary detection +Portalusagereact-use-measuredependency invx-demo)Gallery, and to@vx/tooltipexamplesTesting
@vx/tooltipdocs; note: portal does require additional calculation complexity, but it's not too bad. Hopefully the demo will be good for showing people how to do this.Without Portal
With Portal
Gif
In docs
In gallery
@hshoff @kristw @techniq