jasonkuhrt / react-popover

A smart popover component for React
600 stars 253 forks source link

Does not work anymore with SVG elements #148

Closed dlindenkreuz closed 6 years ago

dlindenkreuz commented 6 years ago

Since 0.5.x, react-popover does not work anymore with a <path> or any other SVG element as target (astonishingly enough, it did before).

From now on, react-popover adds a <div> wrapper around its target which breaks the SVG layout.

return <svg>
  <path d="..." />
  <Popover><path d="..." /></Popover>
</svg>
// → DOM: <svg>...<div><path d="..."></path></div></svg>

Internally, react-popover attaches a ref to the wrapper div. It would be unfeasible to attach the ref directly to <Popover>'s children using React.cloneElement because cloneElement preserves existing refs.

The only easy solution I can see right now is to introduce an optional prop that allows specifying a wrapper element type (string). People with SVG usage scenarios would then use g, everyone else uses div by default. <Popover wrapperElementType="g">

Thoughts?

jasonkuhrt commented 6 years ago

@dlindenkreuz Great find and reporting, thanks! The reason we wrap is to support ref access. We did not do that before because we used ReactDOM.findDOMNode.

Options:

I think option 1 above is the best path forward. A case of not using ref to keep the API simple for consumers is worthwhile IMO. I don't see any downsides currently other than going against the best practice, but again this is a reasonable exception.

jasonkuhrt commented 6 years ago

Once I get repo permissions back I'm going to mark this issue as a bug as it breaks previous behaviour in an unexpected way.

jasonkuhrt commented 6 years ago

@dlindenkreuz Thanks for reporting! Try 0.5.3 that I just published. The solution uses findDOMNode so as a consumer it should JustWork (tm) for you.

dlindenkreuz commented 6 years ago

Sweet, thx for the quick fix!