Submenu nesting and saving new nested items. #21671
Conversation
|
Size Change: -12.2 kB (1%) Total Size: 828 kB
ℹ️ View Unchanged
|
talldan
left a comment
talldan
left a comment
There was a problem hiding this comment.
This looks good, but It'd be great to see some helpful comments as the code is significantly more complicated now.
I also think considering how it can be unit tested soon would be good given the complexity.
packages/edit-navigation/src/components/menu-editor/use-navigation-blocks.js
Outdated
Show resolved
Hide resolved
| } | ||
|
|
||
| menuItemsRef.current = {}; | ||
| const itemIndex = {}; |
There was a problem hiding this comment.
Looking at what this does, it seems like it builds an index of items by their parent, I feel like a name like itemsByParent might document this better.
| return innerBlocks; | ||
| }; | ||
|
|
||
| const innerBlocks = createMenuItemBlocks( itemIndex[ 0 ] ); |
There was a problem hiding this comment.
It's not clear to me why only the item with id 0 is passed in. Would be good to see some comments here.
There was a problem hiding this comment.
If it were itemsByParentID[ 0 ] then I think it'd be clear enough what's going on.
packages/edit-navigation/src/components/menu-editor/use-navigation-blocks.js
Outdated
Show resolved
Hide resolved
| return innerBlocks; | ||
| }; | ||
|
|
||
| const innerBlocks = createMenuItemBlocks( itemIndex[ 0 ] ); |
There was a problem hiding this comment.
If it were itemsByParentID[ 0 ] then I think it'd be clear enough what's going on.
| const innerBlocks = []; | ||
| for ( const item of items ) { | ||
| let menuItemInnerBlocks = []; | ||
| if ( itemIndex[ item.id ] && itemIndex[ item.id ].length > 0 ) { |
There was a problem hiding this comment.
Could use ?. to shorten this. (I'm maybe a little obsessed with using this now that we can...)
if ( itemIndex[ item.id ]?.length ) {
packages/edit-navigation/src/components/menu-editor/use-navigation-blocks.js
Show resolved
Hide resolved
| menuItemsRef.current[ block.clientId ] = menuItem; | ||
| innerBlocks.push( block ); | ||
| } | ||
| const createMenuItemBlocks = ( items ) => { |
There was a problem hiding this comment.
We're going to want to unit test as much of this logic as possible. It's probably easiest to do that if we move the logic that turns a flat map of menu items into a tree of blocks out of the React hook and into a pure function that is name-exported.
function createBlocksFromMenuItems( menuItems ) {
...
}
...
const { blocks, menuItemsByClientId } = createBlocksFromMenuItems( menuItems );
menuItemsRef.current = menuItemsByClientId;
setBlocks( [ createBlock( 'core/navigation', {}, blocks ) ] );This is kind of what I had in mind when I wrote createBlockFromMenuItem, but I didn't really think it through properly.
Happy to defer this for when we do #21285.
packages/edit-navigation/src/components/menu-editor/use-navigation-blocks.js
Outdated
Show resolved
Hide resolved
packages/edit-navigation/src/components/menu-editor/use-navigation-blocks.js
Outdated
Show resolved
Hide resolved
| saveMenuItem( { | ||
| ...menuItem, | ||
| ...createMenuItemAttributesFromBlock( block ), | ||
| menus: menuId, // Gotta do this because REST API doesn't like receiving an array here. Maybe a bug in the REST API? |
There was a problem hiding this comment.
Do we need to set parent here? What happens if a Navigation Link is moved from one submenu to another?
There was a problem hiding this comment.
Good question; it doesn't appear to be possible to move items between nesting levels at the moment. Also, I just realised that nav items can be dragged by the handle from the top toolbar and the experience is... not delightful 🙀
There was a problem hiding this comment.
We should still send parent even if our UI is missing proper nesting, I think!
|
I've addressed all the feedback so ready for another round of review! Regarding saving the menu order, it doesn't seem to be too straightforward so it's probably better to do it in a follow up PR. |
|
Just surfacing the discussion about saving menu items in a transactional way here: |
| // Disable reason, this code will eventually be implemented. | ||
| // eslint-disable-next-line no-unused-vars | ||
| for ( const clientId of deletedClientIds ) { | ||
| for ( const deletedClientId of deletedClientIds ) { |
There was a problem hiding this comment.
👋 let's revert this line as it will conflict with the PR that implements this ( or tries to :) )
There was a problem hiding this comment.
If I do that, it fails the lint check because clientId is already declared in the upper scope. The deletion work will require some reworking to integrate with this PR anyway - which is what I attempted in #22128 😅
| ...createMenuItemAttributesFromBlock( block ), | ||
| menus: menuId, | ||
| parent: parentId, | ||
| } ).then( ( result ) => result ); |
There was a problem hiding this comment.
I added that when I was trying to save reordering, where I needed to know the parent's position in the menu so I could order its children. That turned out to not be possible with the current API endpoints, and I forgot to remove this code.
| saveMenuItem( { | ||
| ...menuItem, | ||
| ...createMenuItemAttributesFromBlock( block ), | ||
| menus: menuId, // Gotta do this because REST API doesn't like receiving an array here. Maybe a bug in the REST API? |
There was a problem hiding this comment.
We should still send parent even if our UI is missing proper nesting, I think!
|
There are some PHP lint errors which is what is causing Travis CI to fail. |
noisysocks
left a comment
noisysocks
left a comment
There was a problem hiding this comment.
Works great! Nice work. Just fix up those PHP lint errors and we're good to go 👍 
| if ( 0 === (int) $prepared_nav_item['menu-item-position'] ) { | ||
| if ( $menu_items ) { | ||
| $last_item = array_pop( $menu_items ); | ||
| $last_item = $menu_items[count( $menu_items ) - 1]; |
There was a problem hiding this comment.
Do we need to update or add a test to phpunit/class-rest-nav-menu-items-controller-test.php to test this change?
Description
Closes #21590 and #21593
Added ability to render submenus as nested, and save nested items, in the experimental navigation page.
TODO: Render new nested items in the correct order (currently they're appended to the end of the list).
How has this been tested?
Tested in browser.
Screenshots
We might need some styling fixes for this 🙀 :
Types of changes
New feature (non-breaking change which adds functionality)
Checklist: