Fix auto Nav menu creation due to page list inner blocks #46223
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
getdave
requested review from
scruffian,
MaggieCabrera,
georgeh,
draganescu and
ajlende
getdave
added
[Type] Regression
|
Size Change: +116 B (0%) Total Size: 1.33 MB
ℹ️ View Unchanged
|
getdave
commented
Dec 1, 2022
Comment on lines
88
to
91
| innerBlocksAreDirty = ! isEqual( | ||
| originalPageListBlockWithoutInnerBlocks, | ||
| currentPageListBlockWithoutInnerBlocks | ||
| ); |
There was a problem hiding this comment.
Referential equality check is not possible here because we are creating two new objects using the de-structuring. Similarly we cannot mutate the original objects [to get only inner blocks] because that would invalidate future checks of originalBlocks.
getdave
commented
Dec 1, 2022
packages/block-library/src/navigation/edit/unsaved-inner-blocks.js
Outdated
Show resolved
Hide resolved
|
Overall I'm not convinced this approach is robust enough. I can't put my finger on why yet but we need to be careful before we merge this one. We could also consider a more generic solution that somehow makes use of |
|
@talldan I wonder if we could consider using the |
This comment was marked as resolved.
This comment was marked as resolved.
|
@georgeh Thank you. This is interesting. Does the same happen for you on |
|
@georgeh what theme are you testing with? |
I think that selector was around when I originally wrote some of this code, and it didn't work or had some caveats. Possibly the issue is that inner blocks need to render before It's hard to say for sure though, as the code has changed a lot and what was once an issue may no longer be an issue. |
getdave
commented
Dec 5, 2022
packages/block-library/src/navigation/edit/unsaved-inner-blocks.js
Outdated
Show resolved
Hide resolved
getdave
commented
Dec 5, 2022
packages/block-library/src/navigation/edit/unsaved-inner-blocks.js
Outdated
Show resolved
Hide resolved
|
Not sure how to proceed here. We need
I'm not super happy with how "hacky" this code feels but it does improve the experience for the user from what we have now which is not great at all with those Menus being auto-created. |
scruffian
approved these changes
Dec 5, 2022
There was a problem hiding this comment.
This works for me. As noted, there are still bugs if the block provides both Page List and other blocks. I think we should merge this can create a follow up issue to consider more cases, but this will cover most scenarios so I wouldn't want to block it on other edge cases.
MaggieCabrera
approved these changes
Dec 5, 2022
mikachan
reviewed
Dec 5, 2022
There was a problem hiding this comment.
I'm seeing the same as @georgeh when I test this.
- Testing with Twenty Twenty-Three
- Deleted all the Navigation Menus
With this PR:
- Inserting a Navigation Block creates an empty block:
- Inserting a Page List creates a 'primary' Navigation Menu on save with all my pages (which I think is correct, just confirming)
On trunk:
- Inserting a Navigation Block creates a 'Primary' Navigation Menu without needing to save the Site Editor or any other trigger. The default page list is my top level pages only.
- Inserting a Page List inserts a list of all my pages, but doesn't create a new menu (maybe it's updating Primary?)
- The nav block performance seems a lot worse on trunk compared to this PR
getdave
commented
Dec 6, 2022
draganescu
reviewed
Dec 6, 2022
packages/block-library/src/navigation/edit/unsaved-inner-blocks.js
Outdated
Show resolved
Hide resolved
draganescu
reviewed
Dec 6, 2022
packages/block-library/src/navigation/edit/unsaved-inner-blocks.js
Outdated
Show resolved
Hide resolved
scruffian
reviewed
Dec 6, 2022
draganescu
approved these changes
Dec 6, 2022
There was a problem hiding this comment.
b0b9a42 this is a lot more elegant, and fixes potential future problems as well
Excellent work and persistence 👏🏻 🙇🏻
|
Thanks for the work. It's good to have a more advanced page list. As far as I can tell, this appears to work as intended — random inner blocks are saved in the post, not in the separate menu CPT, right up until you click the menu at which point a snackbar pops up:
This is probably okay as a bugfix to mitigate a bit of an edgecase. In general, as soon as you change properties of a navigation block, that's when it becomes a custom menu. That's where this edgecase becomes a bit more tricky, because if you don't click the nav menu, those custom inner blocks are still saved inside the nav menu, as inner blocks. They persist, and can appear as if your menu is done, saved, complete. Yet it's not persisted: To be fair, to me this feels also like an edgecase, not necessarily something to optimize for. But I still wonder if there isn't an opportunity here to improve things in the longer term. I'm unaware of the new page list block is built, technically, so I will refer to you on the details. But I keep coming back to the multi entity saving flow — the white dot added to the Save/Publish button when other entities are edited — as being a good interface for this. I understand the main blocker in using this flow for the nav block is that this only works if the entity has first been created, and simply needs to be updated, and that we can't create this entity as part of the flow. Nevertheless, if it was possible to do this, instead of actually creating a menu when uncontrolled innerblocks are detected, we wouldn't have to resort to attaching this action to click instead. Something to think about. |
getdave
commented
Dec 6, 2022
packages/block-library/src/navigation/edit/unsaved-inner-blocks.js
Outdated
Show resolved
Hide resolved
|
As a follow up let's explore using whether the block is "locked" as the factor to determine whether the block's |
Co-authored-by: Andrei Draganescu <[email protected]>
getdave
force-pushed
the
fix/nav-post-creation-due-to-page-list-inner-blocks-changes
branch
from
e68865e to
ec45184
Compare
getdave
deleted the
fix/nav-post-creation-due-to-page-list-inner-blocks-changes
branch
|
@scruffian @draganescu I added some test coverage here #46329 |
|
mpkelly
pushed a commit
to mpkelly/gutenberg
that referenced
this pull request
Dec 7, 2022
…6223) * Add exception for page list to ignore its inner blocks * Explain what is going on * Only consider the first block * Remove debugging * Be specific in the referential equality check comments * Ditch Lodash for Fast Deep Equal * Use customised conditional deep equality * Remove unnecessary let * Add comments for context * Fix grammar Co-authored-by: Andrei Draganescu <[email protected]> * Target fix to Page List only Co-authored-by: Andrei Draganescu <[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.
What?
Fixes bug whereby the fallback Page List in the Navigation block was automatically creating a new Navigation Menu (post) on load.
Why?
This was happening because Page List has now been converted to use InnerBlocks. It is dynamic in the sense that the Page List Items are loaded from the REST API and thus the
.innerBlocksproperty changes as the data is received.innerBlocksis[]innerBlocksis an array of blocksThe
UncontrolledInnerBlockscomponent in the Nav block checks for changes to the uncontrolled inner blocks and if there are any changes it considers the blocks to have become "dirty" and triggers the creation of a Navigation Menu.Unfortunately due to the dynamic population of inner blocks in the Page List block (as described above) this behaviour now caused the auto creation of a menu on insertion.
How?
The fix is currently achieved by adding a specific condition for Page List to ignore the inner blocks and only check for changes to the block's other properties (i.e.: we ignore changes to inner blocks of the Page List block).
This results in the dynamic changes to
innerBlocksbeing ignored and thus the Nav Men is not auto created. However if you make other changes to the Nav block's inner blocks such as:...the Nav Menu will be correctly auto created as before.
Testing Instructions
This needs careful testing.
Before all tests please clear your Nav Menus at http://localhost:8888/wp-admin/edit.php?post_type=wp_navigation.
Test Page List
EditTest Adding New Block
Existing Inner Blocks
Testing Instructions for Keyboard
Screenshots or screencast
Co-authored-by: Maggie <[email protected]>
Co-authored-by: Ben Dwyer <[email protected]>