Remove findDOMNode usage from the NavigableToolbar component #11401
Conversation
youknowriad
added
the
[Type] Code Quality
| @@ -0,0 +1,132 @@ | |||
|
|
|||
There was a problem hiding this comment.
Thank goodness for the new files. These components were always the hardest to find because of their non-obvious prior bundling.
| @@ -0,0 +1,132 @@ | |||
|
|
|||
| } | ||
|
|
||
| const forwardedNavigableContainer = ( props, ref ) => { | ||
| return <NavigableContainer { ...props } forwardedRef={ ref } />; |
There was a problem hiding this comment.
Minor: I think the ownProps pattern we've adopted elsewhere (e.g. withSelect) when we know we're just passing through props directly, to avoid wastefully iterating over the props in the pass-through.
There was a problem hiding this comment.
I don't like that we have to change the inner component's implementation to adapt to this. I find it ok for higher-order components that need to be very performant but personally prefer to avoid it in regular components.
There was a problem hiding this comment.
I'd agree in the sense of not surfacing it on the public API, but it seems perfectly fine (and arguably preferable) for internal pass-through components.
youknowriad
force-pushed
the
remove/find-dom-node-toolbar
branch
from
3cb6e00 to
f4a88cb
Compare
Related #11360
This PR ends up more complex that I though.
forwardRefsupport to the NavigableMenu component