Leverage forwardRef to remove findDOMNode from the block component #11170
Conversation
youknowriad
added
the
[Type] Code Quality
|
Test failures here appear legitimate. |
youknowriad
force-pushed
the
remove/find-dom-node-block
branch
2 times, most recently
from
c54c094 to
ce4bd54
Compare
youknowriad
force-pushed
the
remove/find-dom-node-block
branch
from
ce4bd54 to
acbb4d0
Compare
jorgefilipecosta
left a comment
jorgefilipecosta
left a comment
There was a problem hiding this comment.
Things looked fine in my tests 👍
| } | ||
|
|
||
| export default IgnoreNestedEvents; | ||
| export default forwardRef( ( props, ref ) => { |
|
|
||
| this.wrapperNode = node; | ||
|
|
||
| this.props.blockRef( node, this.props.clientId ); |
There was a problem hiding this comment.
Not related but something I noticed while reviewing. The parent passes the clientId and a callback function blockRef that receives the node and the clientId, passing the clientId here seems unnecessary the parent can pass a normal ref function blockRef specific for each cliendId.
| this.props.blockRef( node, this.props.clientId ); | ||
| } | ||
|
|
||
| bindBlockNode( node ) { |
There was a problem hiding this comment.
For this ref (bindBlockNode), it seems we can use createRef in the constructor. But I'm not sure if it is beneficial or has any advantage besides saving one or two lines of code.
There was a problem hiding this comment.
I think we can probably use forwardRef in the Block component to get directly the ref to the div here and use createRef but Let's not change this in this PR.
youknowriad
force-pushed
the
remove/find-dom-node-block
branch
from
acbb4d0 to
460acf9
Compare
This PR removes
findDOMNodeusage from the block wrapper component by leveragingforwardReffor theIgnoreNestedEventscomponent as we only want to reference its inner DOM Node.