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.

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: Update TinyMCE component to use componentDidUpdate #8148

Closed
wants to merge 1 commit into from

Conversation

aduth
Copy link
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
Copy link
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
Member Author

aduth commented Aug 3, 2018

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

image

@aduth
Copy link
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
12 tasks
@youknowriad
Copy link
Contributor

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
Member

ellatrix commented Nov 1, 2018

Let me have a look as well.

@aduth
Copy link
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
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
Development

Successfully merging this pull request may close these issues.

4 participants