Site editor: do not remount iframe #46431
Merged
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
ellatrix
commented
Dec 9, 2022
| @@ -257,8 +254,6 @@ export default function BlockEditor( { setIsInserterOpen } ) { | |||
| <BlockEditorKeyboardShortcuts.Register /> | |||
| <BackButton /> | |||
| <ResizableEditor | |||
| // Reinitialize the editor and reset the states when the template changes. | |||
| key={ templateId } | |||
There was a problem hiding this comment.
If a reset in size is needed, we should be doing it inside the ResizableEditor components and not remount the whole tree.
|
Size Change: -29 B (0%) Total Size: 1.32 MB
ℹ️ View Unchanged
|
|
Indeed, switching templates is awesome in this PR 👏 Still testing edge cases |
|
The auto-resizing was introduced for Template Parts and their focus mode. It would be a good idea to test those for regressions. |
youknowriad
approved these changes
Dec 9, 2022
|
The focus mode works as before 👍 |
Mamaduka
added
[Package] Edit Site
|
Great! If there's any regression, let's not fix it by changing keys. :) |
|
FWIW, using keys to reset states is a common pattern documented in the official doc. It does remount the component but often times it's not that bad as it sounds. However, after some iterations, I don't think it's needed anymore (we no longer |
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.
What?
This was added by #35974, but I can't see why it's necessary.
I did some testing and I can't see anything breaking.
It sounds like this was added to reset the height of the canvas, but I'm unable to find a case where it's needed.
Even if we need a reset there, we shouldn't be unmounting the whole iframe.
Why?
How?
Testing Instructions
Testing Instructions for Keyboard
Screenshots or screencast