Skip to content

Rich Text: Update TinyMCE component to use componentDidUpdate#8148

Closed
aduth wants to merge 1 commit intomasterfrom
update/tinymce-will-receive-props
Closed

Rich Text: Update TinyMCE component to use componentDidUpdate#8148
aduth wants to merge 1 commit intomasterfrom
update/tinymce-will-receive-props

Conversation

@aduth
Copy link
Copy Markdown
Member

@aduth aduth commented Jul 23, 2018

This pull request seeks to update the TinyMCE component to avoid usage of the deprecated componentWillReceiveProps React function, which will log warnings when using the React.StrictMode component.

Implementation notes:

Since this lifecycle method operates on the bound DOM node, it is a fairly straight-forward conversion. In fact, this probably should have been implemented using componentDidUpdate all along.

There's probably some broader refactoring that could be done here to avoid the manual DOM mutations. This pull request is only meant to address the deprecated lifecycle usage.

Testing instructions:

Verify that there are no regressions in the behavior covered by this method:

  • Placeholder visibility
  • Styles application
  • Classname application
  • Aria key application

@aduth aduth added [Component] TinyMCE [Feature] Rich Text Related to the Rich Text component that allows developers to render a contenteditable [Type] Code Quality Issues or PRs that relate to code quality labels Jul 23, 2018
wpscholar
wpscholar approved these changes Jul 25, 2018
View reviewed changes
Copy link
Copy Markdown
Member

@wpscholar wpscholar left a comment

Choose a reason for hiding this comment

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

Changes look good to me.

@aduth aduth force-pushed the update/tinymce-will-receive-props branch from d80d2b3 to 6edc8ca Compare August 3, 2018 19:08
@aduth
Copy link
Copy Markdown
Member Author

aduth commented Aug 3, 2018

Investigating a strange visual regression when creating a new paragraph...

image

@aduth
Copy link
Copy Markdown
Member Author

aduth commented Aug 3, 2018

Ah, it's obvious to me now why it wouldn't be working. We have shouldComponentUpdate always returning false, so while previously componentWillReceiveProps would have been sufficient for effecting changes, componentDidUpdate is not since the component will never... update.

@aduth aduth added the [Status] In Progress Tracking issues with work in progress label Aug 6, 2018
@ellatrix ellatrix mentioned this pull request Nov 1, 2018
Closed
12 tasks
@youknowriad
Copy link
Copy Markdown
Contributor

youknowriad commented Nov 1, 2018

Yes, I tried this as well, I wonder if this is one of those places where we should just rename the function to the UNSAFE version.

@ellatrix
Copy link
Copy Markdown
Member

ellatrix commented Nov 1, 2018

Let me have a look as well.

@aduth aduth mentioned this pull request Nov 1, 2018
Merged
4 tasks
@aduth
Copy link
Copy Markdown
Member Author

aduth commented Nov 1, 2018

Superseded by #11368

@aduth aduth closed this Nov 1, 2018
@youknowriad youknowriad deleted the update/tinymce-will-receive-props branch November 1, 2018 17:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@wpscholar wpscholar wpscholar approved these changes

Assignees

No one assigned

Labels

[Feature] Rich Text Related to the Rich Text component that allows developers to render a contenteditable [Status] In Progress Tracking issues with work in progress [Type] Code Quality Issues or PRs that relate to code quality

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@aduth @youknowriad @ellatrix @wpscholar