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

Rich Text: Remove componentDidUpdate deep equality conditions #7648

Closed
wants to merge 2 commits into from
Closed

Rich Text: Remove componentDidUpdate deep equality conditions #7648

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
084328e
Rich Text: Remove componentDidUpdate deep equality
aduth Jun 30, 2018
ce4cb90
Rich Text: Scold deeply equally referentially inequal value updates
aduth Jul 3, 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
27 changes: 17 additions & 10 deletions editor/components/rich-text/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -681,27 +681,34 @@ export class RichText extends Component {
}

componentDidUpdate( prevProps ) {
const { tagName, value } = this.props;

// The `savedContent` var allows us to avoid updating the content right after an `onChange` call
if (
!! this.editor &&
this.props.tagName === prevProps.tagName &&
this.props.value !== prevProps.value &&
this.props.value !== this.savedContent &&

// Comparing using isEqual is necessary especially to avoid unnecessary updateContent calls
// This fixes issues in multi richText blocks like quotes when moving the focus between
// the different editables.
! isEqual( this.props.value, prevProps.value ) &&
! isEqual( this.props.value, this.savedContent )
Copy link
Member

Choose a reason for hiding this comment

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

Would there be any point in keeping the check to log a warning in dev mode if values are the same but an update is triggered?

Copy link
Member Author

Choose a reason for hiding this comment

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

Would there be any point in keeping the check to log a warning in dev mode if values are the same but an update is triggered?

I love that idea!

Copy link
Member Author

Choose a reason for hiding this comment

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

Scolding added in 320c96e7108fbc6fd1e536656ee2304ae32ece49

tagName === prevProps.tagName &&
value !== prevProps.value &&
value !== this.savedContent
) {
this.setContent( this.props.value );
}

// Note: Environment condition must be independent for it to be dropped
// in dead code elimination with webpack.DefinePlugin.
if ( 'development' === process.env.NODE_ENV ) {
/* eslint-disable no-console */
if ( value !== prevProps.value && isEqual( value, prevProps.value ) ) {
console.error( 'RichText value reference changed while being deeply equal to `prevProps.value`. This will incur wasted effort to update the RichText field.' );
}

if ( value !== this.savedContent && isEqual( value, this.savedContent ) ) {
console.error( 'RichText value reference changed while being deeply equal to `this.savedContent`. This will incur wasted effort to update the RichText field.' );
}

if ( ! isEqual( this.props.formatters, prevProps.formatters ) ) {
// eslint-disable-next-line no-console
console.error( 'Formatters passed via `formatters` prop will only be registered once. Formatters can be enabled/disabled via the `formattingControls` prop.' );
}
/* eslint-enable no-console */
}
}

Expand Down
6 changes: 3 additions & 3 deletions test/e2e/specs/__snapshots__/adding-blocks.test.js.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ exports[`adding blocks Should insert content using the placeholder and the regul
<!-- /wp:paragraph -->

<!-- wp:quote -->
<blockquote class=\\"wp-block-quote\\"><p>Quote block</p></blockquote>
<blockquote class=\\"wp-block-quote\\"><p>Quote block</p><cite>Quote citation</cite></blockquote>
<!-- /wp:quote -->

<!-- wp:image -->
Expand All @@ -24,7 +24,7 @@ exports[`adding blocks Should insert content using the placeholder and the regul
<!-- /wp:paragraph -->

<!-- wp:quote -->
<blockquote class=\\"wp-block-quote\\"><p>Quote block</p></blockquote>
<blockquote class=\\"wp-block-quote\\"><p>Quote block</p><cite>Quote citation</cite></blockquote>
<!-- /wp:quote -->"
`;

Expand All @@ -38,7 +38,7 @@ exports[`adding blocks Should insert content using the placeholder and the regul
<!-- /wp:paragraph -->

<!-- wp:quote -->
<blockquote class=\\"wp-block-quote\\"><p>Quote block</p></blockquote>
<blockquote class=\\"wp-block-quote\\"><p>Quote block</p><cite>Quote citation</cite></blockquote>
<!-- /wp:quote -->

<!-- wp:preformatted -->
Expand Down
8 changes: 2 additions & 6 deletions test/e2e/specs/adding-blocks.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,10 +39,11 @@ describe( 'adding blocks', () => {
await page.keyboard.type( '/quote' );
await page.keyboard.press( 'Enter' );
await page.keyboard.type( 'Quote block' );
await page.keyboard.press( 'ArrowDown' );
await page.keyboard.type( 'Quote citation' );

// Arrow down into default appender.
await page.keyboard.press( 'ArrowDown' );
await page.keyboard.press( 'ArrowDown' );

// Focus should be moved to block focus boundary on a block which does
// not have its own inputs (e.g. image). Proceeding to press enter will
Expand Down Expand Up @@ -88,11 +89,6 @@ describe( 'adding blocks', () => {
await page.click( '[data-type="core/quote"] .editor-block-list__insertion-point-button' );
await page.keyboard.type( 'Second paragraph' );

// Switch to Text Mode to check HTML Output
await page.click( '.edit-post-more-menu [aria-label="More"]' );
const codeEditorButton = ( await page.$x( '//button[contains(text(), \'Code Editor\')]' ) )[ 0 ];
await codeEditorButton.click( 'button' );

expect( await getEditedPostContent() ).toMatchSnapshot();
} );
} );