Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Leverage forwardRef to remove findDOMNode from the block component #11170

Merged
merged 4 commits into from
Nov 2, 2018
Merged

Leverage forwardRef to remove findDOMNode from the block component #11170

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
03170d9
Leverage forwardRef to remove findDOMNode from the block component
youknowriad Oct 28, 2018
b39f48b
Update changelog
youknowriad Oct 31, 2018
111d823
Fix unit tests
youknowriad Oct 31, 2018
460acf9
Add a display name
youknowriad Nov 2, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions packages/editor/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
### Polish

- Remove `findDOMNode` usage from the `Inserter` component.
- Remove `findDOMNode` usage from the `Block` component.

## 6.1.0 (2018-10-30)

Expand Down
16 changes: 2 additions & 14 deletions packages/editor/src/components/block-list/block.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ import { get, reduce, size, first, last } from 'lodash';
/**
* WordPress dependencies
*/
import { Component, findDOMNode, Fragment } from '@wordpress/element';
import { Component, Fragment } from '@wordpress/element';
import {
focus,
isTextField,
Expand Down Expand Up @@ -102,24 +102,12 @@ export class BlockListBlock extends Component {
}

setBlockListRef( node ) {
// Disable reason: The root return element uses a component to manage
// event nesting, but the parent block list layout needs the raw DOM
// node to track multi-selection.
//
// eslint-disable-next-line react/no-find-dom-node
node = findDOMNode( node );

this.wrapperNode = node;

this.props.blockRef( node, this.props.clientId );
Copy link
Member

@jorgefilipecosta jorgefilipecosta Nov 1, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}

bindBlockNode( node ) {
Copy link
Member

@jorgefilipecosta jorgefilipecosta Nov 1, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

// Disable reason: The block element uses a component to manage event
// nesting, but we rely on a raw DOM node for focusing.
//
// eslint-disable-next-line react/no-find-dom-node
this.node = findDOMNode( node );
this.node = node;
}

/**
Expand Down
15 changes: 10 additions & 5 deletions packages/editor/src/components/ignore-nested-events/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ import { reduce } from 'lodash';
/**
* WordPress dependencies
*/
import { Component } from '@wordpress/element';
import { Component, forwardRef } from '@wordpress/element';

/**
* Component which renders a div with passed props applied except the optional
Expand All @@ -22,7 +22,7 @@ import { Component } from '@wordpress/element';
*
* @type {Component}
*/
class IgnoreNestedEvents extends Component {
export class IgnoreNestedEvents extends Component {
constructor() {
super( ...arguments );

Expand Down Expand Up @@ -66,7 +66,7 @@ class IgnoreNestedEvents extends Component {
}

render() {
const { childHandledEvents = [], ...props } = this.props;
const { childHandledEvents = [], forwardedRef, ...props } = this.props;

const eventHandlers = reduce( [
...childHandledEvents,
Expand Down Expand Up @@ -96,8 +96,13 @@ class IgnoreNestedEvents extends Component {
return result;
}, {} );

return <div { ...props } { ...eventHandlers } />;
return <div ref={ forwardedRef } { ...props } { ...eventHandlers } />;
}
}

export default IgnoreNestedEvents;
const forwardedIgnoreNestedEvents = ( props, ref ) => {
return <IgnoreNestedEvents { ...props } forwardedRef={ ref } />;
};
forwardedIgnoreNestedEvents.displayName = 'IgnoreNestedEvents';

export default forwardRef( forwardedIgnoreNestedEvents );
2 changes: 1 addition & 1 deletion packages/editor/src/components/ignore-nested-events/test/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ import { mount } from 'enzyme';
/**
* Internal dependencies
*/
import IgnoreNestedEvents from '../';
import { IgnoreNestedEvents } from '../';

describe( 'IgnoreNestedEvents', () => {
it( 'passes props to its rendered div', () => {
Expand Down