Ensure the overlay menu works when inserting navigation block patterns and that inner blocks are retained #37358
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
talldan
added
[Type] Bug
talldan
commented
Dec 14, 2021
|
Size Change: +136 B (0%) Total Size: 1.13 MB
ℹ️ View Unchanged
|
talldan
changed the title
talldan
force-pushed
the
fix/overlay-menu-for-unsaved-inner-blocks
branch
from
31da4ee to
ce0901c
Compare
talldan
commented
Dec 15, 2021
| expect( await getNavigationMenuRawContent() ).toMatchSnapshot(); | ||
| } ); | ||
|
|
||
| describe( 'Creating and restarting', () => { |
There was a problem hiding this comment.
This is the end-to-end test added in #36880. It was skipped, but I've fixed and re-enabled it here so that it asserts that the changes from that PR still work.
There was a problem hiding this comment.
Noted these don't provide full coverage for the issue uncovered in #37358 (review). Hopefully simple to add that coverage though?
getdave
reviewed
Dec 15, 2021
There was a problem hiding this comment.
Thank you for working on these issues.
It fixes the Core issues you reference 🥳
However...unfortunately I found this PR regresses #36880.
- New Post
- Switch to code view and paste in:
<!-- wp:navigation {"overlayMenu":"none"} -->
<!-- wp:navigation-link {"label":"Hello world!","type":"post","id":1,"url":"http://localhost:8888/?p=1","kind":"post-type","isTopLevelLink":true} /-->
<!-- wp:navigation-link {"label":"Sample Page","type":"page","id":2,"url":"http://localhost:8888/?page_id=2","kind":"post-type","isTopLevelLink":true} /-->
<!-- /wp:navigation -->
- Update without switching out of code view.
- Switch to Visual Mode and select the Nav block - it will be converted to controlled blocks.
- Click
Select Menu -> Create New - Observe the broken UI state with an infinite spinner.
- Check
wp.data.select('core/block-editor').getBlocks()and look for the Nav block. See that it still has inner blocks.
I have tried to push a fix which manually updates the innerblocks in the block editor store.
I'd prefer this fix was separated out from the overlay menu so it's easier to revert as required, but at this stage it's not a blocker.
getdave
reviewed
Dec 15, 2021
|
Added an e2e test for the fix, thanks for pushing a fix. As you say in the comment, blocks shouldn't be concerned about this, but I think this is the first use case of a core block that has both uncontrolled and controlled blocks. |
talldan
force-pushed
the
fix/overlay-menu-for-unsaved-inner-blocks
branch
from
885f9a0 to
afa6931
Compare
getdave
approved these changes
Dec 16, 2021
|
Tested it on Wednesday and both issue cases seems to work now. Big Thank you. 🙂 |
tellthemachines
removed
the
Backport to WP 6.7 Beta/RC
tellthemachines
pushed a commit
that referenced
this pull request
Dec 21, 2021
…s and that inner blocks are retained (#37358) * Show overlay menu for unsaved inner blocks too * Fix uncontrolled inner blocks being deleted * Add e2e test for converting markup inner blocks to a navigation post * Change useNavigationMenu back to trunk * Re-enable single entity test * Attempt to sync block editor store with controlled state Addresses https://github.com/WordPress/gutenberg/pull/37358\#discussion_r769699131 * Add e2e test for lingering uncontrolled blocks issue Co-authored-by: Dave Smith <[email protected]>
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 #36670
Fixes #37354
This fixes the two issues mentioned above. When testing the first one, I noticed the second one was no longer working.
The first issue is that the
UnsavedInnerBlockscomponent that's shown when the block has standard uncontrolled inner blocks (this is hard to describe without using technical terms) didn't use theResponsiveWrappercomponent that's used for showing the overlay menu. That was a pretty simple fix.The second issue was introduced when another fix went in recently (#36880). It's understandable as the
UnsavedInnerBlocksbehavior there isn't all that easy to follow, it exists to support an edge case, and also there are no end to end tests.How has this been tested?
Also test that the fix in #36880 continues to work.
Types of changes
Bug fix (non-breaking change which fixes an issue)
Checklist:
*.native.jsfiles for terms that need renaming or removal).