Fix Nav block editing wrong entity on creation of new Menu #36880
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
getdave
added
[Block] Navigation
|
Size Change: -21 B (0%) Total Size: 1.1 MB
ℹ️ View Unchanged
|
This was referenced Nov 25, 2021
talldan
reviewed
Nov 26, 2021
getdave
force-pushed
the
fix/nav-block-updating-wrong-entity-record
branch
from
259db3b
to
93246ac
Compare
|
Failing e2e test appears to be around dirty entities which worries me. I'll run locally to check... |
I think we've introduced a bug here with dirty entity states as the test is failing because the |
|
I added an additional check for |
getdave
commented
Nov 26, 2021
| } | ||
| // innerBlocks are intentionally not listed as deps. This function is only concerned | ||
| // with the snapshot from the time when ref became undefined. | ||
| }, [ clientId, ref ] ); |
There was a problem hiding this comment.
Suggested change
| }, [ clientId, ref ] ); | |
| }, [ clientId, ref, innerBlocks ] ); |
Do we now need to add innerBlocks to the deps?
There was a problem hiding this comment.
No, as the comment above states :-)
There was a problem hiding this comment.
Oops, looks like it was added anyway in 793ff2a
There was a problem hiding this comment.
I don't even know what happened here 😕
|
@adamziel this now looks good. e2e passes. Needs a 👍 pending one final query in #36880 (comment) |
adamziel
approved these changes
Nov 26, 2021
noisysocks
removed
the
Backport to WP Beta/RC
noisysocks
pushed a commit
that referenced
this pull request
Nov 29, 2021
* Move reset of innerblocks to effect * Add e2e test that checks for updating the wrong entity record * Formatting * Unfocus focused test * Only replace innerBlocks with an empty list, if it's not already empty * Update useEffect dependencies Co-authored-by: Adam Zieliński <adam@adamziel.com> Co-authored-by: Daniel Richards <daniel.richards@automattic.com>
talldan
reviewed
Dec 14, 2021
This was referenced Dec 14, 2021
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
In #36862 we learnt that if you use the
Create newbuttons within the Nav block UI to create a new Menu you can end up causing edits to the wrongwp_navigationentity.How does this happen? Scenario:
Create newdrop theSelect menudropdown on the blockwp_navigationentity for editing.replaceInnerBlocksbut it does this before the entity is set to the freshly created one. Instead it ends up calling replaceBlocks and the existing entity picks up the changes and gets marked as "dirty"Savein the Site Editor only to find they now have to save both the new and the original Nav Post entitiesThis PR fixes that by moving the reseting of the inner blocks to an effect which runs if the
refto thewp_navigationPost is reset. This way we ensure we have completely "unloaded" the current entity before we reset the blocks in the UI.Closes #36862
How has this been tested?
Select menu -> Create new.Screenshots
Types of changes
Bug fix (non-breaking change which fixes an issue)
Checklist:
*.native.jsfiles for terms that need renaming or removal).