withTooltip enhancer can't be used on components that render within an svg

[ptmx]

The withTooltip enhancer from @vx/tooltip currently adds a container div that wraps the component: https://github.com/hshoff/vx/blob/f8e6c6d20dd146de8fbf3879aa00806619ad0ffc/packages/vx-tooltip/src/enhancers/withTooltip.tsx#L75-L87

Since HTML elements can't be appended to SVG elements, this means that the withTooltip enhancer can't be used on components that will be rendered within an SVG, such as an Axis.

I made a minimal example of a reusable AxisBottomWithTooltip component that adds tooltips to axis tick labels, but I have a similar use case in a charting library I'm building for a project.

In the example above, I modified withTooltip to render the wrapped component directly, without inserting the container div, so the axis renders correctly with the tooltip even though it's rendered within an SVG element.

Using the withTooltip enhancer from @vx/tooltip instead, nothing is rendered due to the container div.

The downside of removing the container div is that it would be a breaking change for any existing code which relies on the container, so users would have to migrate by adding such a container around their enhanced component instead. From my point of view that seems like the more flexible approach, but I might be missing some context on why it's helpful for the container div to be added by withTooltip.

[williaster]

Hey @ptmx 👋 thanks for checking out vx and raising this issue. I think you point is definitely fair and there could be a use for using withTooltip with components that are children of an svg. I would point out that an alternative implementation of your axis tooltip using the current withTooltip (with wrapper div) is to simply pass the showTooltip + hideTooltip from the parent svg down to the child Axis component.

Still, your proposal could enable more flexibility. I wonder if rather than making a breaking change and removing the div container altogether if we could instead add a hook to render the container element, which would allow a consumer to render no container if they wanted (and render the current div with necessary position: 'relative' style by default). wdyt of that approach?

[ptmx]

Hey @ptmx 👋 thanks for checking out vx and raising this issue. I think you point is definitely fair and there could be a use for using withTooltip with components that are children of an svg. I would point out that an alternative implementation of your axis tooltip using the current withTooltip (with wrapper div) is to simply pass the showTooltip + hideTooltip from the parent svg down to the child Axis component.

Hi @williaster 👋! Thanks for the reply. Yeah, for a one-off case it's definitely possible to use withTooltip on the parent component and pass down the tooltip props to the axis component. For my use case, since I want the tooltip behavior to be a part of the reusable axis component, it's better if I can encapsulate that behavior in the component itself without having to pass in the tooltip props.

Still, your proposal could enable more flexibility. I wonder if rather than making a breaking change and removing the div container altogether if we could instead add a hook to render the container element, which would allow a consumer to render no container if they wanted (and render the current div with necessary position: 'relative' style by default). wdyt of that approach?

That approach makes sense to me - I'll follow up on the corresponding PR (#610) this week.

I'll likely also make a useTooltip hook for my own purposes - happy to contribute that as well but don't want to introduce too many different patterns since the codebase currently uses render props and HoC throughout. I can PR it if I go down that road and let y'all decide 😄.

[williaster]

That all makes sense and sounds good 👍 Excited to hear about the interest in a useTooltip hook PR as well, that would be hugely useful! 🚀

For more context on hooks, we had been holding off on them to maintain compatibility with older versions of react since they require 16.7+, and concentrated on the TS re-write + some preparations for a v1 launch. However we are definitely interested in supporting them, and think it's a good time to start adding them / bumping react version requirements per-package as needed 🎉

[ptmx]

Closed via #631