Refactor Nav creation hooks and associated code #38824

getdave · 2022-02-15T14:25:19Z

Description

Continues work on #37190.

This PR looks to address the inconsistencies around the code paths (especially hooks) around the creation of classic menus into block based menus.

Currently many of the hooks involved are very opaque and despite being async, provide little or not information about the resolution status of the async requests involved. This causes UI issues such as that which is experienced when selecting a classic menu from the dropdown and observing how the UI appears "locked" whilst the async request resolves.

This PR takes a step towards solving these problems. Principally it exposes information about the resolution status of the useConvertClassicMenu hook which allows the UI to respond according.

Testing Instructions

Screenshots

Types of changes

Checklist:

My code is tested.
My code follows the WordPress code style.
My code follows the accessibility standards.
I've tested my changes with keyboard and screen readers.
My code has proper inline documentation.
I've included developer documentation if appropriate.
I've updated all React Native files affected by any refactorings/renamings in this PR (please manually search all *.native.js files for terms that need renaming or removal).
I've updated related schemas if appropriate.

getdave · 2022-02-15T14:26:25Z

packages/block-library/src/navigation/edit/index.js

@@ -495,6 +528,13 @@ function Navigation( {
 											setRef( id );
 											onClose();
 										} }
+										onSelectClassicMenu={ ( menu ) => {
+											onClose();


Calling onClose here allows the dropdown to close and the block UI to update whilst the async conversion process happens.

Need to be a little careful about focus management here.

getdave · 2022-02-15T14:28:05Z

packages/block-library/src/navigation/edit/index.js

@@ -495,6 +528,13 @@ function Navigation( {
 											setRef( id );
 											onClose();
 										} }
+										onSelectClassicMenu={ ( menu ) => {
+											onClose();
+											convertClassicMenuToBlocks(


By passing the conversion code down into the dropdown we avoid errors thrown when the dropdown unmounts and an async request triggered by that component resolves. We do this by handling in the parent (which is not unmounted when the option is clicked).

getdave · 2022-02-15T14:28:44Z

packages/block-library/src/navigation/edit/index.js

@@ -676,6 +716,7 @@ function Navigation( {
 						/>
 					) }
 					{ ! hasResolvedCanUserCreateNavigation ||
+						isResolvingClassicMenuConversion ||
 						( ! isEntityAvailable && ! isPlaceholderShown && (
 							<PlaceholderPreview isLoading />


We need to look again at loading states because I don't yet see this. Perhaps because of the call to await createNavigationMenu (which again has no resolution status indicators).

getdave · 2022-02-15T14:32:51Z

packages/block-library/src/navigation/use-convert-classic-menu.js

@@ -55,4 +59,12 @@ export default function useConvertClassicMenu( onFinish ) {
 		},
 		[ hasResolvedMenuItems, createFromMenu ]
 	);
+
+	return {
+		dispatch,


I don't love dispatch here. We could use run or similar?

talldan

Dropping an early review, though I realise this is still a draft.

I'm pretty familiar with this code as I had to write this last minute patch for 5.9 - #38168.

I didn't do anything drastic in that PR, but it did introduce the useConvertClassicMenu hook, which to be honest, I don't love. It was just moving the existing code into something reusable rather than improving the code quality.

IMO, I think we should probably explore more drastic changes than we do in this PR and instead try to get rid of useConvertClassicMenu.

Here's what I think is the problem. Hooks that work with or return data are generally 'long lived', they'll be active for entire lifecycle of a component. Converting a menu though is a one-time action that doesn't require data to be kept around.

There are two bits of data that useConvertClassicMenu persists when it doesn't need to - the classic menu name, and the classic menu items (which it fetches once it has a menu id). I think that's problematic, because what are we supposed to do with this data after a conversion has happened? We could reset it, but then we're just doing garbage collection manually.

Another option is to instead make classic menu conversion a normal async function instead of a hook. Give the function a menu id, it maybe calls apiFetch instead of useSelect to get the menu items and returns blocks. The only thing we lose is caching of the classic menu items in the store, but I don't think classic menu conversion is a common or regular interaction so it's an ok trade-off.

It may also be good to get some design input into the loading states. I don't know how we can adequately convey that clicking a classic menu is an asynchronous operation.

talldan · 2022-02-16T03:47:48Z

packages/block-library/src/navigation/use-convert-classic-menu.js

@@ -55,4 +59,12 @@ export default function useConvertClassicMenu( onFinish ) {
 		},
 		[ hasResolvedMenuItems, createFromMenu ]
 	);
+
+	return {
+		dispatch,


talldan · 2022-02-16T03:51:22Z

packages/block-library/src/navigation/edit/index.js

+	const {
+		dispatch: convertClassicMenuToBlocks,
+		blocks: classicMenuBlocks,
+		name: classicMenuName,
+		isResolving: isResolvingClassicMenuConversion,
+		hasResolved: hasResolvedClassicMenuConversion,
+	} = useConvertClassicMenu();


It seems like a lot of overhead to run this on every render of edit when converting classic menus is a rare interaction for the user. It'll result in the block always fetching all classic menus.

talldan · 2022-02-16T04:17:35Z

packages/block-library/src/navigation/edit/index.js

+		if (
+			hasResolvedClassicMenuConversion &&
+			classicMenuName &&
+			classicMenuBlocks?.length


If I have an empty classic menu, nothing happens when clicking the menu item. (it's the first thing I noticed when testing as all my classic test menus are empty 😄 )

talldan · 2022-02-16T04:23:56Z

packages/block-library/src/navigation/edit/index.js

@@ -495,6 +528,13 @@ function Navigation( {
 											setRef( id );
 											onClose();
 										} }
+										onSelectClassicMenu={ ( menu ) => {
+											onClose();


Need to be a little careful about focus management here.

talldan · 2022-02-16T04:30:34Z

packages/block-library/src/navigation/edit/placeholder/index.js

+		hasResolvedClassicMenuConversion,
+	] );
+
+	const isStillLoading =


I know this was already in trunk, but I think a good improvement would be a rename of this variable:

Suggested change

const isStillLoading =

const isLoading =

getdave · 2022-02-16T11:29:48Z

Dropping an early review, though I realise this is still a draft.

I'm pretty familiar with this code as I had to write this last minute patch for 5.9 - #38168.

I didn't do anything drastic in that PR, but it did introduce the useConvertClassicMenu hook, which to be honest, I don't love. It was just moving the existing code into something reusable rather than improving the code quality.

IMO, I think we should probably explore more drastic changes than we do in this PR and instead try to get rid of useConvertClassicMenu.

Here's what I think is the problem. Hooks that work with or return data are generally 'long lived', they'll be active for entire lifecycle of a component. Converting a menu though is a one-time action that doesn't require data to be kept around.

There are two bits of data that useConvertClassicMenu persists when it doesn't need to - the classic menu name, and the classic menu items (which it fetches once it has a menu id). I think that's problematic, because what are we supposed to do with this data after a conversion has happened? We could reset it, but then we're just doing garbage collection manually.

Another option is to instead make classic menu conversion a normal async function instead of a hook. Give the function a menu id, it maybe calls apiFetch instead of useSelect to get the menu items and returns blocks. The only thing we lose is caching of the classic menu items in the store, but I don't think classic menu conversion is a common or regular interaction so it's an ok trade-off.

It may also be good to get some design input into the loading states. I don't know how we can adequately convey that clicking a classic menu is an asynchronous operation.

TBH I've been going round and round with this trying to decide what to do. I thought I'd push this up and at least then it would be easier to discuss. Glad you 👀 and left thoughts - thank you.

Let's consider the more drastic changes you suggest. I am keen that we are able to convey when something is "working" so we'll need to keep that in mind.

getdave · 2022-02-16T13:14:45Z

I don't know how we can adequately convey that clicking a classic menu is an asynchronous operation.

For what it's worth I don't think we can or should attempt to do this within the dropdown / menu item itself. Instead we

Click the menu item.
Immediately close the menu - this is expected UX else it feels like the UI is freezing/locking up.
Trigger a "loading" UI in the block itself.
Once menu conversion is resolved, disable the loading state.

The block shouldn't care how the data is being loaded. All it needs to know is that the data isn't available and thus it should be in a "loading" state.

adamziel · 2022-02-16T13:57:44Z

Another option is to instead make classic menu conversion a normal async function instead of a hook. Give the function a menu id, it maybe calls apiFetch instead of useSelect to get the menu items and returns blocks. The only thing we lose is caching of the classic menu items in the store, but I don't think classic menu conversion is a common or regular interaction so it's an ok trade-off.

We can lean on select even if that's an async function if we use the registry:

const registry = useRegistry();
const callback = useCallback(() => {
    setIsConverting( true );
    try{
        await convertClassicMenu( registry );
        showSuccessNotice();
    } catch(e) {
        showErrorNotice(e);
    } finally {
        setIsConverting( false );
    }
}, [registry])

and then we can do things like registry.resolveSelect( coreStore ).getEntityRecords( /* ... */ )

getdave · 2022-02-16T14:29:35Z

Another option is to instead make classic menu conversion a normal async function instead of a hook. Give the function a menu id, it maybe calls apiFetch instead of useSelect to get the menu items and returns blocks. The only thing we lose is caching of the classic menu items in the store, but I don't think classic menu conversion is a common or regular interaction so it's an ok trade-off.

We can lean on select even if that's an async function if we use the registry:

const registry = useRegistry();
const callback = useCallback(() => {
    setIsConverting( true );
    try{
        await convertClassicMenu( registry );
        showSuccessNotice();
    } catch(e) {
        showErrorNotice(e);
    } finally {
        setIsConverting( false );
    }
}, [registry])

and then we can do things like registry.resolveSelect( coreStore ).getEntityRecords( /* ... */ )

😮 Thanks Adam!

I have another PR where I've changed course to use "one big async function". I'll push that up and then post a link here so we can discuss if/how to migrate over to using registry and friends as @adamziel suggests.

getdave · 2022-02-16T15:42:09Z

Here's a draft PR for the alternative approach.

getdave · 2022-02-21T13:49:09Z

Closing in favour of #38858

getdave added 4 commits February 15, 2022 11:04

Refactor hook to expose data

62234f5

Refactor away classic menu creation code

b8bc7da

Call menu conversion callback with correct args

a046fd5

Use identical hooks in placeholder

a9e8980

getdave self-assigned this Feb 15, 2022

getdave added the [Block] Navigation Affects the Navigation Block label Feb 15, 2022

getdave commented Feb 15, 2022

View reviewed changes

Remove callback

fa71d7c

getdave commented Feb 15, 2022

View reviewed changes

Rename exposed as run

8fec011

talldan reviewed Feb 16, 2022

View reviewed changes

getdave mentioned this pull request Feb 16, 2022

Refactor classic menu conversion process #38858

Merged

14 tasks

getdave closed this Feb 21, 2022

talldan mentioned this pull request Aug 4, 2022

Fix navigation select menu focus loss #42956

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor Nav creation hooks and associated code #38824

Refactor Nav creation hooks and associated code #38824

getdave commented Feb 15, 2022

getdave Feb 15, 2022

talldan Feb 16, 2022

getdave Feb 15, 2022

getdave Feb 15, 2022

getdave Feb 15, 2022

talldan Feb 16, 2022

talldan left a comment

talldan Feb 16, 2022

talldan Feb 16, 2022

talldan Feb 16, 2022

talldan Feb 16, 2022

talldan Feb 16, 2022

getdave commented Feb 16, 2022

getdave commented Feb 16, 2022

adamziel commented Feb 16, 2022 •

edited

Loading

getdave commented Feb 16, 2022

getdave commented Feb 16, 2022

getdave commented Feb 21, 2022

Refactor Nav creation hooks and associated code #38824

Refactor Nav creation hooks and associated code #38824

Conversation

getdave commented Feb 15, 2022

Description

Testing Instructions

Screenshots

Types of changes

Checklist:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

talldan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

getdave commented Feb 16, 2022

getdave commented Feb 16, 2022

adamziel commented Feb 16, 2022 • edited Loading

getdave commented Feb 16, 2022

getdave commented Feb 16, 2022

getdave commented Feb 21, 2022

adamziel commented Feb 16, 2022 •

edited

Loading