Add useTooltip hook #631
Add useTooltip hook #631
Conversation
| tooltipOpen, | ||
| showTooltip, | ||
| hideTooltip, | ||
| } = useTooltip(); |
There was a problem hiding this comment.
Is it possible to create a “Tooltip” class and return a tooltip instance with the hook? const [tooltip, updateTooltip] = useTooltip() feels more consistent to how people would use other hooks. It’s also more scalable in case we want to add more API to the tooltip.
There was a problem hiding this comment.
I think returning an object of the same props that are passed via withTooltip makes sense.
I've seen several examples of hooks that use this pattern (e.g., react-three-fiber's useThree hook), and it would avoid a breaking change for withTooltip.
There was a problem hiding this comment.
This looks great to me @ptmx! 🙌 Thanks for the addition of THE FIRST HOOK, for refactoring the HOC, and for such great docs! 💯
I think refactoring the HOC to use the hook internally is the right move. The only reason I can think of not to do this is if there are significant perf differences between HOCs + hooks, but I've never looked into this.
Hopefully just the start of hooks for vx 👯
| showTooltip, | ||
| }: ShowProvidedProps & WithTooltipProvidedProps<TooltipData>) => { | ||
| }: ShowProvidedProps) => { | ||
| const { |
There was a problem hiding this comment.
thanks for updating the source file, too. working on an improvement to not need to duplicate source + this string (I've tried a few things but if you have any thoughts lmk 😄 )
There was a problem hiding this comment.
This stuck out to me as something to come back to as well. It might be doable with raw-loader or raw.macro but I didn't have time to dig into it more deeply. Now this has me curious so I'll look into it later in the week if I have a chance 🤔
There was a problem hiding this comment.
sounds good! Created #632 to track this, I'll update status there if I start work on it. raw.macro looks interesting, linked to how react-window does it in the issue as well 👍
| tooltipOpen, | ||
| showTooltip, | ||
| hideTooltip, | ||
| } = useTooltip(); |
There was a problem hiding this comment.
I think returning an object of the same props that are passed via withTooltip makes sense.
I've seen several examples of hooks that use this pattern (e.g., react-three-fiber's useThree hook), and it would avoid a breaking change for withTooltip.
| containerProps, | ||
| ); | ||
| } | ||
| return renderContainer(<BaseComponent {...props} {...tooltipProps} />, containerProps); |
There was a problem hiding this comment.
just noting that previously we spread the WrappedComponent props after tooltipProps, so this might result in a change in behavior if a consumer did pass an overlapping prop. I'm not really sure this is a big concern, but would prevent doing things like overriding tooltipOpen 🤷♂ any thoughts on this?
There was a problem hiding this comment.
Ah, good catch! I agree it's not super likely that any consumers are relying on that behavior now, but in the absence of a reason to change it I think it makes sense to stay consistent with the existing behavior. Will update shortly.
| type UpdateTooltipArgs<TooltipData> = UseTooltipState<TooltipData>; | ||
|
|
||
| export default function useTooltip<TooltipData = {}>(): UseTooltipParams<TooltipData> { | ||
| const [{ tooltipOpen, tooltipLeft, tooltipTop, tooltipData }, setTooltipState] = useState< |
There was a problem hiding this comment.
Since they're mostly updated together, I agree that a single useState with all state is better than storing each variable separately in different hooks 👍
|
this is now published under |
🚀 Enhancements
useTooltiphook to manage tooltip state for functional components, as discussed in withTooltip enhancer can't be used on components that render within an svg #609💥 Breaking Changes
useState, which requires bumping thepeerDepfor react to^16.8.0-0📝 Documentation
vx-tooltipreadme to include the newuseTooltiphook, and recommend a state management approach depending on use case (specifically: recommendinguseTooltipfor functional components, orwithTooltipfor class components)BarStackdemo to useuseTooltipinstead ofwithTooltip, so there is an example of each approach to managing tooltip state in the gallery (BarStackforuseTooltip,BarStackHorizontalforwithTooltip)🏠 Internal
withTooltipin terms ofuseTooltip, to avoid maintaining multiple different implementations