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

Eliminate obsolete call to onChange when RichText componentWillUnmount #6150

Merged
merged 1 commit into from
Apr 13, 2018

Conversation

westonruter
Copy link
Member

Fixes #6134.

When wp.hooks.addFilter( 'blocks.BlockEdit'… ) is used to extend blocks which contain the RichEdit component, this will cause the component to be unmounted and with that onChange will be called:

componentWillUnmount() {
this.onChange();
}

This results in the editor being put into a dirty state upon initialization, even if no changes have been made by the user. This bit of code actually appears to be a remnant of the past, @iseulde in #6134 (comment):

Everything should be synced whenever there is a change in the editor. At first sight I think it is fine to remove. It seems to be a remnant from when we only synced the value on blur.

But @youknowriad adds:

I agree with this statement, but I'd test carefully the different usecases, of splitting/merging/switching etc... :) fast typing + splitting

I'm not sure how best to go about testing this change, but I wanted to open a PR at least to get the fix off the ground.

@gziolo gziolo added this to the 2.7 milestone Apr 13, 2018
@gziolo gziolo added [Type] Bug An existing feature does not function as intended [Feature] Rich Text Related to the Rich Text component that allows developers to render a contenteditable labels Apr 13, 2018
@gziolo
Copy link
Member

gziolo commented Apr 13, 2018

I like this change. Hopefully, it doesn't introduce any regressions :)

Copy link
Contributor

@youknowriad youknowriad left a comment

Choose a reason for hiding this comment

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

LGTM 👍 No regression found. If we encounter one, it will be an excuse to add an E2E test :)

@gziolo gziolo merged commit 4c0521e into master Apr 13, 2018
@gziolo gziolo deleted the fix/unmount-onchange branch April 13, 2018 12:23
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 [Type] Bug An existing feature does not function as intended
Projects
None yet
Development

Successfully merging this pull request may close these issues.

RichText onChange is erroneously called (and puts editor in dirty state) when wrapped in withFilters HOC
4 participants