Simplfy handling of save of Nav block uncontrolled inner blocks #45517
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
[Type] Enhancement
|
|
getdave
commented
Nov 3, 2022
| @@ -191,9 +191,6 @@ function Navigation( { | |||
| __unstableMarkNextChangeAsNotPersistent, | |||
| } = useDispatch( blockEditorStore ); | |||
|
|
|||
| const [ hasSavedUnsavedInnerBlocks, setHasSavedUnsavedInnerBlocks ] = | |||
There was a problem hiding this comment.
We shouldn't need this state in the parent.
There was a problem hiding this comment.
Yeah, agree I don't think it's needed. I think it was only there to support the previous behavior where a menu would be created whenever UnsavedInnerBlocks mounted. From memory there where issues where UnsavedInnerBlocks would be unmounted and remounted and could cause duplicate menus to be created—the parent block needed a way to guard the menu creation process.
Now this only happens when a user selects/modifies the inner blocks, and I don't think as much guarding is needed. 👍
I think it's worth giving this a lot of manual testing to be sure, as it's hard to reason about some aspects of the nav block code and also core data resolution.
getdave
commented
Nov 3, 2022
| @@ -646,7 +649,8 @@ function Navigation( { | |||
| // Consider this state as 'unsaved' and offer an uncontrolled version of inner blocks, | |||
| // that automatically saves the menu as an entity when changes are made to the inner blocks. | |||
| const hasUnsavedBlocks = hasUncontrolledInnerBlocks && ! isEntityAvailable; | |||
| if ( hasUnsavedBlocks ) { | |||
|
|
|||
| if ( hasUnsavedBlocks && ! isCreatingNavigationMenu ) { | |||
There was a problem hiding this comment.
Addition of ! isCreatingNavigationMenu is important here. We shouldn't render the uncontrolled inner blocks if a menu is being created. That way we can defer all the UI logic for a "loading" or "saving" state to the main edit.js component.
getdave
commented
Nov 3, 2022
| @@ -703,24 +707,11 @@ function Navigation( { | |||
| overlayTextColor={ overlayTextColor } | |||
| > | |||
| <UnsavedInnerBlocks | |||
| createNavigationMenu={ createNavigationMenu } | |||
There was a problem hiding this comment.
Here's where we pass down the primary creation function.
getdave
commented
Nov 3, 2022
Comment on lines
-714
to
-764
| onSave={ ( post ) => { | ||
| // Set some state used as a guard to prevent the creation of multiple posts. | ||
| setHasSavedUnsavedInnerBlocks( true ); | ||
| // Switch to using the wp_navigation entity. | ||
| setRef( post.id ); | ||
|
|
||
| showNavigationMenuStatusNotice( | ||
| __( `New Navigation Menu created.` ) | ||
| ); | ||
| } } |
There was a problem hiding this comment.
This is now all defer to the centralised logic within the edit component which handles creation of menus. Saves maintaining another set of "loading" logic.
getdave
commented
Nov 3, 2022
Comment on lines
-175
to
-183
| { isSaving ? ( | ||
| <Spinner | ||
| className={ | ||
| 'wp-block-navigation__uncontrolled-inner-blocks-loading-indicator' | ||
| } | ||
| /> | ||
| ) : ( | ||
| <Wrapper { ...innerBlocksProps } /> | ||
| ) } |
There was a problem hiding this comment.
We no longer require this logic as the component will not be rendered when creating a menu.
getdave
requested review from
draganescu,
MaggieCabrera,
scruffian,
talldan and
ajlende
getdave
added
[Type] Code Quality
4 tasks
|
Size Change: -112 B (0%) Total Size: 1.29 MB
ℹ️ View Unchanged
|
draganescu
approved these changes
Nov 8, 2022
talldan
reviewed
Nov 8, 2022
talldan
reviewed
Nov 8, 2022
Co-authored-by: Daniel Richards <daniel.richards@automattic.com>
getdave
force-pushed
the
try/simplify-nav-uncontrolled-inner-blocks
branch
from
5e4395e
to
b0aecfa
Compare
talldan
approved these changes
Nov 9, 2022
|
Waiting on CI tests 🕐 |
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?
Simplifies and centralising the "saving" logic and rendering for uncontrolled inner blocks by defering to the implementation in the parent component (primary
edit.jsof the block).Why?
Having a separate usage of
createNavigationMenuwithinuncontroller-inner-blocks.jswas requiring the block to maintain a separate "saving" or "loading" state when creating menus from uncontroller inner blocks (for example converting a Page List into a menu).This was leading to potential UI bugs and also greatly increasing the complexity of the code within
<UnControlledInnerBlocks>.This problem was encountered when working on #45443.
How?
With this change we pass in the "create" function and defer all UI handling for saving/loading to the parent component.
Benefits:
<UncontrolledInnerBlocks>is no longer rendered if a menu is being created avoiding the need to maintain separeate "loading" UI within that component.UncontrolledInnerBlocksaround "loading" statessetStatecall in the parentedit.jswhich simplifies that implementationTesting Instructions
The main thing to check is the conversion of inner blocks to sync'd menu.
Delete all Navigation Menus & Classic Menus - http://localhost:8888/wp-admin/edit.php?post_type=wp_navigation
New Post
Add Nav block
Click on Page List block and click "Edit". Then "Convert".
See loading state whilst conversion takes place.
See a single Menu created.
Delete all Navigation Menus & Classic Menus - http://localhost:8888/wp-admin/edit.php?post_type=wp_navigation
New Post
Go to Code view mode and paste in the following:
Screenshots or screencast