search

jasonkuhrt / react-popover

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

Remove useless div around this.props.children #150

Closed Kerumen closed 7 years ago

Kerumen commented 7 years ago

Introduced in the React 16 support, an enclosing div was set around this.props.children. It broke the styling of our app as we don't have a way to style this enclosing div.

By using cloneElement no extra markup is generated.

jasonkuhrt commented 7 years ago

Related to #148 which goes into a bit more detail about the problem.

Kerumen commented 7 years ago

@jasonkuhrt Yes! But in our app the issue was not about SVG. You mentioned several fixes for this issue. What about this PR? The behavior will be exactly the same as before and you will have the ref access.

jasonkuhrt commented 7 years ago

@Kerumen Whoops thought I was commenting on an issue. Thanks for the PR!

This approach won't work if children does not support ref I think? Also what would be the advantage of this approach over using ReactDOM.findDOMNode?

Kerumen commented 7 years ago

This approach won't work if children does not support ref I think?

I don't know. What kind of children doesn't support ref?

Also what would be the advantage of this approach over using ReactDOM.findDOMNode?

Nothing. But I don't know how can you make findDOMNode works with the array in the render.

jasonkuhrt commented 7 years ago

What kind of children doesn't support ref?

Functional components for example. I found the docs here instructive: https://reactjs.org/docs/refs-and-the-dom.html#exposing-dom-refs-to-parent-components.

And attaching ref to custom components might not do what you expect:

While you could add a ref to the child component, this is not an ideal solution, as you would only get a component instance rather than a DOM node. Additionally, this wouldn’t work with functional components.

Quite a few paragraphs later:

All things considered, we advise against exposing DOM nodes whenever possible, but this can be a useful escape hatch. Note that this approach requires you to add some code to the child component. If you have absolutely no control over the child component implementation, your last option is to use findDOMNode(), but it is discouraged.

I believe findDOMNode is the best solution for us in this case.

jasonkuhrt commented 7 years ago

Superseded by #151.

jasonkuhrt commented 7 years ago

@Kerumen Thanks for your time on this anyways!