Navigation: Create nav menu on pattern insertion or when the block has uncontrolled inner blocks #36024
Conversation
talldan
added
[Type] Enhancement
|
A situation where there are lots of created menus is a little too easy to create IMO. Especially if the user is just trying out patterns. I'm thinking we need a way to automatically clean up Maybe we can look into drafts or auto-drafts, and if the associated menu is still in draft status and the block is removed, it could be deleted? I'm thinking a draft menu also wouldn't be selectable from another instance of a nav block. This system might be challenging though because the main entity saving flow in the editor would need to know to migrate the post status from 'draft' to 'published'. |
|
Size Change: +1.27 kB (0%) Total Size: 1.08 MB
ℹ️ View Unchanged
|
|
Another option could be to explore the entity having an unsaved in-memory state. Saving this via the main entity flow would publish it. It might be a longer-term solution though. |
|
Thanks for this. Just user experience wise, this vastly improves the experience of inserting patterns, and the fact that I can click one of the menus from the pattern and swap it out with one of my existing menus, that just shows the sheer power of what this can be:
That's the amazing potency of persisted menus: insert a pattern, wire it up, and you're done! And it will sync. The downside is all the saved menus created as I'm just paging through patterns. I imagine that to be made worse as you initially set up your theme and flip through patterns using the template part pagination. Can we hold off on saving until you're actually ready to save? |
|
Inspired by this one, I created mockups in #35947 (comment) which all force you to save the menu in order to save the post/page. |
I think it could be even better if the menu were wired up from the start, which is what we could do after #36002. Patterns that are for headers could specify the 'header' area, and automatically pull in the menu the user has assigned to the header. If there's no menu assigned, the fallback would be specifying the inner blocks as we are now.
If this UI uses a block preview (which I think it does), it won't create a menu until inserted.
I think possibly we can try a few things:
|
talldan
force-pushed
the
try/creating-nav-menu-on-pattern-insertion
branch
from
54eeed2 to
868ae0a
Compare
|
Have iterated further on this with the following changes:
There's one glitchy bit that I'd still like to fix, and that's after inserting the pattern, selecting the block causes it to momentarily disappear and reappear. That's because it's being saved asynchronously and the inner blocks are being switched from uncontrolled to controlled. But I think this is open to more feedback, if you can just ignore that bit for now 😄 |
|
Took it for a quick spin, this is what I see: This vastly improves the experience of using patterns, nice work. Can we do the same for from-scratch navigation menus? Instead of having to save the menu before adding blocks to it, can it happen after the fact? That would be along the lines of this mockup, which adds the multi entity saving dot as soon as you click "Start empty". And so long as the dot is there, you can't leave the page until you've saved the menu. |
|
For now, until we find a better way to auto-name (maybe once we're more clear about navigation area names?) I am in support of the naming modal. The crux is that by making naming obvious you actually empower the menu switching feature, because the user has pause to pick a name. Automattic naming is easy on creation but impossible later, which menu is "Untitled menu 2"? I have seen enough GDrives filled with "Untitled document", enough to fear this pattern. If there is no nudge to name menus, people will work with the defaults. People always work with the defaults. The result will be in effect we're making menus unusable instead of reusable. Auto-naming or naming on save, all good, but for now this is a good stepping stone IMO until the better options have good solutions. |
talldan
force-pushed
the
try/creating-nav-menu-on-pattern-insertion
branch
from
868ae0a to
37448a9
Compare
This part should be resolved now. |
| if ( | ||
| hasSavedUnsavedInnerBlocks || | ||
| isDisabled || | ||
| isSaving || | ||
| savingLock.current || | ||
| ! hasResolvedDraftNavigationMenus || | ||
| ! hasResolvedNavigationMenus || | ||
| ! hasSelection | ||
| ) { |
There was a problem hiding this comment.
I find the amount that's going on here a bit bonkers, but I don't have another solution. There's some extreme care needed to ensure the menus are only created once.
Fix issues with multiple menu creation Lift limit on loading of menus Remove attempt to use template part area to generate a name Remove unused client id prop Try creating as auto-draft and publishing on save Only save when selecting menu Show a draft save button on the block toolbar Show loading state when navigation block first loads Remove flicker when selecting block with unsaved inner blocks Polish switch to controlled inner blocks Take drafts into account when naming menus Fix repeated line
talldan
force-pushed
the
try/creating-nav-menu-on-pattern-insertion
branch
from
4b06da7 to
86d3e43
Compare
packages/block-library/src/navigation/edit/unsaved-inner-blocks.js
Outdated
Show resolved
Hide resolved
|
This works very well in my testing. The one part that I found weird was that the auto saved menu was a "draft" but it was rendered on the front end anyway. As @talldan explained this is due to the fact that it's invisible to the user from the UI that the navigation is a draft, so a user would expect to see it renderd on the front end. The question then becomes if "draft" is a good status? Maybe a custom status would do better, as draft should be unpublished. |
Not sure what the options are here, but it seems there's no other post status that represents public, but not published. It seems like this would entail a new built-in WordPress post status, something like 'auto-generated' 😬 Or potentially we could use a different mechanism other than status. |
|
One thing I'm noticing is there always seem to be unsaved changes in the Site Editor in this branch. Not sure if it's due to these changes, or some other issue that has since been fixed on trunk. |
@tellthemachines What are the steps to reproduce? |
|
Testing more methodically I now realise the unsaved changes only happen if an unsaved nav block is selected. So steps are:
The above behaviour is specific to this branch - probably because the nav is being saved as a draft behind the scenes? But I'm seeing another save-related issue on trunk:
|
That is to be expected, since clicking into the nav block creates a An improvement might be to create the post when the user makes an edit, but I'm yet to find a good way to do that. I can look into it some more, but I don't think it should be a blocker. |
andrewserong
changed the title
| hasSelection={ isSelected || isInnerBlockSelected } | ||
| hasSavedUnsavedInnerBlocks={ hasSavedUnsavedInnerBlocks } | ||
| onSave={ ( post ) => { | ||
| setHasSavedUnsavedInnerBlocks( true ); |
There was a problem hiding this comment.
Where do we reset the value of this flag? Aren't we supposed to reset it somewhere to "false"?
Description
Iteration on #35947
As discussed on that issue, the process of having the user manually create a menu was considered undesireable.
This PR automatically creates an untitled wp_navigation menu whenever:
An idea discussed on that issue was to automatically use the template part area to name a menu, but was pretty complicated. The template part block often doesn't store the 'area', so the code would have to get all parent template part blocks, get the entities, and then get the nearest area. Template part ids are also a weird combination of 'theme' + 'slug', which seems to be something codified in the block.
How has this been tested?
Screenshots
Types of changes
New feature (non-breaking change which adds functionality)
Checklist:
*.native.jsfiles for terms that need renaming or removal).