Skip to content

Submenu nesting and saving new nested items. #21671

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
tellthemachines merged 8 commits into from
May 13, 2020
Merged

Submenu nesting and saving new nested items. #21671

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
602c2ae
Submenu nesting and saving new nested items.
tellthemachines Apr 17, 2020
d71243b
Address review feedback
tellthemachines Apr 28, 2020
eed638c
Save parent items before nested items
tellthemachines Apr 29, 2020
45c5d6a
Save sub-items of new items
tellthemachines Apr 30, 2020
8ad3ea8
Remove logging in error.
tellthemachines Apr 30, 2020
817902a
Array manipulation improvement
tellthemachines May 1, 2020
51ff1a5
Address review feedback
tellthemachines May 12, 2020
38f7f7e
Fix PHP lint errors
tellthemachines May 13, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions lib/class-wp-rest-menu-items-controller.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -440,12 +440,13 @@ protected function prepare_item_for_database( $request ) {
$menu_items = wp_get_nav_menu_items( $prepared_nav_item['menu-id'], array( 'post_status' => 'publish,draft' ) );
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 ];
if ( $last_item && isset( $last_item->menu_order ) ) {
$prepared_nav_item['menu-item-position'] = $last_item->menu_order + 1;
} else {
$prepared_nav_item['menu-item-position'] = count( $menu_items );
$prepared_nav_item['menu-item-position'] = count( $menu_items ) - 1;
}
array_push( $menu_items, $last_item );
} else {
$prepared_nav_item['menu-item-position'] = 1;
}
Expand Down
114 changes: 75 additions & 39 deletions packages/edit-navigation/src/components/menu-editor/use-navigation-blocks.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
/**
* External dependencies
*/
import { isEqual, difference } from 'lodash';
import { groupBy, isEqual, difference } from 'lodash';

/**
* WordPress dependencies
Expand All @@ -10,11 +10,15 @@ import { createBlock } from '@wordpress/blocks';
import { useSelect, useDispatch } from '@wordpress/data';
import { useState, useRef, useEffect } from '@wordpress/element';

function createBlockFromMenuItem( menuItem ) {
return createBlock( 'core/navigation-link', {
label: menuItem.title.raw,
url: menuItem.url,
} );
function createBlockFromMenuItem( menuItem, innerBlocks = [] ) {
return createBlock(
'core/navigation-link',
{
label: menuItem.title.rendered,
url: menuItem.url,
},
innerBlocks
);
}

function createMenuItemAttributesFromBlock( block ) {
Expand All @@ -25,8 +29,10 @@ function createMenuItemAttributesFromBlock( block ) {
}

export default function useNavigationBlocks( menuId ) {
// menuItems is an array of menu item objects.
const menuItems = useSelect(
( select ) => select( 'core' ).getMenuItems( { menus: menuId } ),
( select ) =>
select( 'core' ).getMenuItems( { menus: menuId, per_page: -1 } ),
[ menuId ]
);

Expand All @@ -41,46 +47,76 @@ export default function useNavigationBlocks( menuId ) {
return;
}

menuItemsRef.current = {};
const itemsByParentID = groupBy( menuItems, 'parent' );

const innerBlocks = [];
menuItemsRef.current = {};

for ( const menuItem of menuItems ) {
const block = createBlockFromMenuItem( menuItem );
menuItemsRef.current[ block.clientId ] = menuItem;
innerBlocks.push( block );
}
const createMenuItemBlocks = ( items ) => {
Copy link
Member

@noisysocks noisysocks Apr 27, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

const innerBlocks = [];
for ( const item of items ) {
let menuItemInnerBlocks = [];
if ( itemsByParentID[ item.id ]?.length ) {
menuItemInnerBlocks = createMenuItemBlocks(
itemsByParentID[ item.id ]
);
}
const block = createBlockFromMenuItem(
item,
menuItemInnerBlocks
);
menuItemsRef.current[ block.clientId ] = item;
innerBlocks.push( block );
}
return innerBlocks;
};

// createMenuItemBlocks takes an array of top-level menu items and recursively creates all their innerBlocks
const innerBlocks = createMenuItemBlocks( itemsByParentID[ 0 ] );
setBlocks( [ createBlock( 'core/navigation', {}, innerBlocks ) ] );
}, [ menuItems ] );

const saveBlocks = () => {
const { innerBlocks } = blocks[ 0 ];

for ( const block of innerBlocks ) {
const menuItem = menuItemsRef.current[ block.clientId ];

if ( ! menuItem ) {
saveMenuItem( {
...createMenuItemAttributesFromBlock( block ),
menus: menuId,
} );
continue;
const { clientId, innerBlocks } = blocks[ 0 ];
const parentItemId = menuItemsRef.current[ clientId ]?.parent;

const saveNestedBlocks = async ( nestedBlocks, parentId = 0 ) => {
for ( const block of nestedBlocks ) {
const menuItem = menuItemsRef.current[ block.clientId ];
let currentItemId = menuItem?.id || 0;

if ( ! menuItem ) {
const savedItem = await saveMenuItem( {
...createMenuItemAttributesFromBlock( block ),
menus: menuId,
parent: parentId,
} );
if ( block.innerBlocks.length ) {
currentItemId = savedItem.id;
}
}

if (
menuItem &&
! isEqual(
block.attributes,
createBlockFromMenuItem( menuItem ).attributes
)
) {
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?
Copy link
Member

@noisysocks noisysocks Apr 27, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need to set parent here? What happens if a Navigation Link is moved from one submenu to another?

Copy link
Contributor Author

@tellthemachines tellthemachines Apr 28, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 🙀

Copy link
Contributor

@draganescu draganescu May 12, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should still send parent even if our UI is missing proper nesting, I think!

parent: parentId,
} );
}

if ( block.innerBlocks.length ) {
saveNestedBlocks( block.innerBlocks, currentItemId );
}
}
};

if (
! isEqual(
block.attributes,
createBlockFromMenuItem( menuItem ).attributes
)
) {
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?
} );
}
}
saveNestedBlocks( innerBlocks, parentItemId );

const deletedClientIds = difference(
Object.keys( menuItemsRef.current ),
Expand All @@ -89,7 +125,7 @@ export default function useNavigationBlocks( menuId ) {

// 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 ) {
Copy link
Contributor

@draganescu draganescu May 12, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👋 let's revert this line as it will conflict with the PR that implements this ( or tries to :) )

Copy link
Contributor Author

@tellthemachines tellthemachines May 12, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 😅

// TODO - delete menu items.
}
};
Expand Down