Fix edited Classic block's content deletion when switching to Code editor #23927
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ntsekouras
added
[Type] Bug
|
Size Change: -1.02 MB (88%) 🏆 Total Size: 1.15 MB
ℹ️ View Unchanged
|
youknowriad
added
the
Backport to WP 6.7 Beta/RC
|
Can you run a performance comparison between the base branch and this one, per Testing Overview § Performance Testing? Thanks! |
ntsekouras
force-pushed
the
fix/classic-block-edited-content-removed-when-switch-code-editor
branch
from
a334fba
to
69d7604
Compare
|
Performance seems to not be affected negatively. Even more seems to be a tiny bit faster now. |
ntsekouras
force-pushed
the
fix/classic-block-edited-content-removed-when-switch-code-editor
branch
from
69d7604
to
10a6bec
Compare
|
@mcsf The performance test actually runs on each PR now, unfortunately, the job doesn't post the result as a comment or something, so you'd have to dig in the github action to see the results. |
|
The performance test don't rely on classic blocks though 🤷 |
D'oh, I'm silly. Unconsciously I probably thought "RichText" instead of "Classic". |
ntsekouras
force-pushed
the
fix/classic-block-edited-content-removed-when-switch-code-editor
branch
from
405ae38
to
1f8ef41
Compare
mcsf
reviewed
Jul 15, 2020
| // We need this check because when we remove the editor (onUnmount) | ||
| // due to the usage of `debounce`, this callback is executed in | ||
| // another tick. This results in setting the content to empty. | ||
| if ( editor._isRemoved ) return; |
There was a problem hiding this comment.
I personally like this style, but I'm surprised the linter didn't complain about this if statement with no curly braces:
if ( A ) {
return;
}There was a problem hiding this comment.
It might have allowSingleLine set. I'll check the rules.
There was a problem hiding this comment.
It seems that allows the omission of curly braces when a block contains only one statement by default, if not explicitly set. I'm not sure yet though.
There are other similar cases in the codebase as well. So if this is a problem, we should check and fix the linter config/rules.
There was a problem hiding this comment.
Yeah, apparently it's not enforced in code, but for reference here are the guidelines:
ellatrix
requested changes
Jul 16, 2020
ellatrix
approved these changes
Jul 16, 2020
ntsekouras
deleted the
fix/classic-block-edited-content-removed-when-switch-code-editor
branch
6 tasks
ellatrix
pushed a commit
that referenced
this pull request
Jul 20, 2020
…itor (#23927) * remove lodash debounce * handle unmount with debounce for performance reasons * add debounce time * utilize lodash debounce cancel method
youknowriad
removed
the
Backport to WP 6.7 Beta/RC
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Fixes: #23924
If you edit the content of a
Classicblock and then switch toCode editor, the content disappears.This was happening because with the switch ( and possibly other actions as well? ) the
Classicblock unmounts and calls thewp.oldEditor( tinymce ) functionremove. This function in a series of internal actions withsaveended up calling our debounced function. Since it was set to execute to anothertick, the editor had already cleared its content (caused by the aboveremovecall) resulting in the disappearance of the content.There might be some performance issues on "edge" cases, as previously discussed here: #23408 (comment), in the PR that introduced this side effect.
Checklist: