Try always generating navigation post title #36760
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
|
Size Change: -179 B (0%) Total Size: 1.1 MB
ℹ️ View Unchanged
|
talldan
reviewed
Nov 23, 2021
Comment on lines
108
to
110
| if ( ! title ) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
Testing and this will return undefined here because there's now no title. It looks like the way this is currently works in the PR is to trigger showing the UnsavedInnerBlocks component and have that create the wp_navigation post. I really think it'd be best to rely less on UnsavedInnerBlocks for that—that component was designed to handle a specific backwards compatibility edge case for patterns and not a whole lot else.
I feel like it'd be better to refactor or duplicate the code that generates a title in UnsavedInnerBlocks so that it can be used in the Placeholder too. That way there's always a title when we call createNavigationMenu and the change is a bit less involved.
It shouldn't be too much to achieve that. There's a few prequisites like calling the useTemplatePartName hook in the Placeholder component to get the area name. And also getting the entire list of menus so that the number added to the end of the name is correct. This bit of code below does the naming, it could be moved into a shared function or hook so that it can be used by both UnsavedInnerBlocks and NavigationPlaceholder:
gutenberg/packages/block-library/src/navigation/edit/unsaved-inner-blocks.js
Lines 137 to 161 in 88883a3
There was a problem hiding this comment.
I added useCreateNavigationMenu that can be reused in both places. The process doesn't fully work yet and there is some flickering involved, but it's one step closer now. cc @tellthemachines
There was a problem hiding this comment.
Thanks @adamziel ! I fixed all the bugs I could find and everything seems to be working correctly now. Would appreciate some extra testing 😄
talldan
added
the
Backport to WP 6.7 Beta/RC
adamziel
reviewed
Nov 23, 2021
packages/block-library/src/navigation/edit/use-default-navigation-title.js
Outdated
Show resolved
Hide resolved
7 tasks
|
All comments have been addressed and features are working as intended, so I'm marking this ready for review. |
getdave
requested changes
Nov 24, 2021
There was a problem hiding this comment.
Thank you for all the work that's going into this. I gave it a spin and found the following issues:
Blank Menu
Creating a new Navigation via Select Menu -> Create New results in a blank menu being created.
- Delete all Navigations.
- Go to Site Editor and add Nav block.
- Click
Start emptyand don't add any blocks. - No click
Select menuagain - notice there's a blank menu entry. - Delete your Nav block.
- Re-add a new Nav block. Notice how
Select menuappears. - Notice how under
Select menuthere is a blank menu entry. - Go to the
wp_navigationPost screen. See that no Posts have been created. It appears this blank entry is a phantom.
Note this phantom entry persists even after you have added items to your Nav block and saved the resulting menu.
Editing Menu in Trash
- Create a Nav block and add some items.
- Save.
- Go to
/wp-admin/edit.php?post_type=wp_navigationand trash all your Navigations. - Go back to the Site Editor.
- See that your Navigation items are still loaded into the Nav block even though they are in trash.
- Add a new item to the Navigation.
- Save.
- See how the item gets restored from Trash and is back to Publish state.
Creating new Nav persists current Nav items
- Create a Navigation block and add some items.
- Save the Template so it is persisted.
- Go to Nav block and click
Select menu -> Create new. - Notice how a fresh Navigation Post is created but the same items are carried across from the current Navigation.
Instead you should receive an entirely blank Navigation with no items.
getdave
reviewed
Nov 24, 2021
| @@ -356,6 +355,7 @@ function Navigation( { | |||
| onClose(); | |||
| } } | |||
| onCreateNew={ () => { | |||
| replaceInnerBlocks( clientId, [] ); | |||
There was a problem hiding this comment.
I added this because when you clicked "Create new menu" it would use the current inner blocks (i.e. from the current menu) and automatically create a new menu from those.
What we want is actually to remove all the inner blocks and reset in order that the placeholder will be shown.
|
|
Previously you could get into an error state if the Nav block referenced a Nav Post which had been deleted. You could not recover from this as the UI provided no way to "start over". This addition affords that ability.
adamziel
force-pushed
the
try/generated-nav-name
branch
from
74c49ad to
1885a87
Compare
adamziel
approved these changes
Nov 24, 2021
There was a problem hiding this comment.
I think this works well 🚢 . There are now three different "create empty" buttons that do at least two different things:
Let's address that in a follow up PR. Great collaboration @getdave @tellthemachines !
getdave
approved these changes
Nov 24, 2021
noisysocks
removed
the
Backport to WP 6.7 Beta/RC
noisysocks
pushed a commit
that referenced
this pull request
Nov 29, 2021
* Try always generating navigation title * Show placeholder preview * Add useCreateNavigationMenu * Update packages/block-library/src/navigation/edit/use-default-navigation-title.js * Fix placeholder and remove unecessary logic. * Pass clientId to placeholder. * Reset inner blocks when creating new menu * Only consider published Navigations as valid * Correct bug with checking status of Nav post * UX to allow creating new menu if associated Nav post not found Previously you could get into an error state if the Nav block referenced a Nav Post which had been deleted. You could not recover from this as the UI provided no way to "start over". This addition affords that ability. * Switch to useGenerateDefaultNavigationTitle * Wrap createNewMenu in useCallback * Rename createNewMenu to startWithEmptyMenu Co-authored-by: Adam Zieliński <[email protected]> Co-authored-by: Dave Smith <[email protected]>
talldan
reviewed
Dec 14, 2021
Comment on lines
+322
to
+335
| if ( navigationMenuId && isNavigationMenuMissing ) { | ||
| return ( | ||
| <div { ...blockProps }> | ||
| <Warning> | ||
| { __( | ||
| 'Navigation menu has been deleted or is unavailable. ' | ||
| ) } | ||
| <Button onClick={ startWithEmptyMenu } variant="link"> | ||
| { __( 'Create a new menu?' ) } | ||
| </Button> | ||
| </Warning> | ||
| </div> | ||
| ); | ||
| } |
There was a problem hiding this comment.
This was removed in #36507, I'm not sure why it made a reappearance here.
Comment on lines
+29
to
+33
| // getEditedEntityRecord will return the post regardless of status. | ||
| // Therefore if the found post is not published then we should ignore it. | ||
| if ( navigationMenu?.status !== 'publish' ) { | ||
| navigationMenu = null; | ||
| } |
There was a problem hiding this comment.
Navigation menus can be drafts too, that's an important part of the pattern insertion flow. Just wondering what the reasoning behind this change was?
82 tasks
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 #36593 and #36575.
Fully automates the Navigation post type naming process. When creating a new Navigation block, user doesn't have to give it a title or save it before starting to edit. It should get saved as part of the editor saving process.
How has this been tested?
Screenshots
Types of changes
Checklist:
*.native.jsfiles for terms that need renaming or removal).