Fix create menu after menu switch (#59630)

* don't set creating menu flag on menu switch * return Promises from the navigation menu ref changing functions to be able to clean state after ref is updated * update tests * await the menu creation * stub test updates * revert test changes and remove decorative promises * do not disable the UI when changing the navigation, disable the UI based on resolving menus * block the UI for menu switching too, update tests with some comments, undo the focus update * properly disable menu choices via their choices prop Co-authored-by: draganescu <andraganescu@git.wordpress.org> Co-authored-by: scruffian <scruffian@git.wordpress.org> Co-authored-by: jeryj <jeryj@git.wordpress.org> Co-authored-by: getdave <get_dave@git.wordpress.org> Co-authored-by: afercia <afercia@git.wordpress.org>
WordPress · Mar 11, 2024 · 86e1422 · 86e1422
1 parent e9266d7
commit 86e1422
Show file tree

Hide file tree

Showing 3 changed files with 97 additions and 57 deletions.
diff --git a/packages/block-library/src/navigation/edit/index.js b/packages/block-library/src/navigation/edit/index.js
@@ -153,8 +153,8 @@ function Navigation( {
 		isError: createNavigationMenuIsError,
 	} = useCreateNavigationMenu( clientId );
 
-	const createUntitledEmptyNavigationMenu = () => {
-		createNavigationMenu( '' );
+	const createUntitledEmptyNavigationMenu = async () => {
+		await createNavigationMenu( '' );
 	};
 
 	const {

diff --git a/packages/block-library/src/navigation/edit/navigation-menu-selector.js b/packages/block-library/src/navigation/edit/navigation-menu-selector.js
@@ -49,7 +49,7 @@ function NavigationMenuSelector( {
 	/* translators: %s: The name of a menu. */
 	const createActionLabel = __( "Create from '%s'" );
 
-	const [ isCreatingMenu, setIsCreatingMenu ] = useState( false );
+	const [ isUpdatingMenuRef, setIsUpdatingMenuRef ] = useState( false );
 
 	actionLabel = actionLabel || createActionLabel;
 
@@ -82,10 +82,20 @@ function NavigationMenuSelector( {
 					value: id,
 					label,
 					ariaLabel: sprintf( actionLabel, label ),
+					disabled:
+						isUpdatingMenuRef ||
+						isResolvingNavigationMenus ||
+						! hasResolvedNavigationMenus,
 				};
 			} ) || []
 		);
-	}, [ navigationMenus, actionLabel ] );
+	}, [
+		navigationMenus,
+		actionLabel,
+		isResolvingNavigationMenus,
+		hasResolvedNavigationMenus,
+		isUpdatingMenuRef,
+	] );
 
 	const hasNavigationMenus = !! navigationMenus?.length;
 	const hasClassicMenus = !! classicMenus?.length;
@@ -99,7 +109,7 @@ function NavigationMenuSelector( {
 
 	let selectorLabel = '';
 
-	if ( isCreatingMenu || isResolvingNavigationMenus ) {
+	if ( isResolvingNavigationMenus ) {
 		selectorLabel = __( 'Loading…' );
 	} else if ( noMenuSelected || noBlockMenus || menuUnavailable ) {
 		// Note: classic Menus may be available.
@@ -111,17 +121,17 @@ function NavigationMenuSelector( {
 
 	useEffect( () => {
 		if (
-			isCreatingMenu &&
+			isUpdatingMenuRef &&
 			( createNavigationMenuIsSuccess || createNavigationMenuIsError )
 		) {
-			setIsCreatingMenu( false );
+			setIsUpdatingMenuRef( false );
 		}
 	}, [
 		hasResolvedNavigationMenus,
 		createNavigationMenuIsSuccess,
 		canUserCreateNavigationMenu,
 		createNavigationMenuIsError,
-		isCreatingMenu,
+		isUpdatingMenuRef,
 		menuUnavailable,
 		noBlockMenus,
 		noMenuSelected,
@@ -140,12 +150,10 @@ function NavigationMenuSelector( {
 							<MenuItemsChoice
 								value={ currentMenuId }
 								onSelect={ ( menuId ) => {
-									setIsCreatingMenu( true );
 									onSelectNavigationMenu( menuId );
 									onClose();
 								} }
 								choices={ menuChoices }
-								disabled={ isCreatingMenu }
 							/>
 						</MenuGroup>
 					) }
@@ -155,17 +163,22 @@ function NavigationMenuSelector( {
 								const label = decodeEntities( menu.name );
 								return (
 									<MenuItem
-										onClick={ () => {
-											setIsCreatingMenu( true );
-											onSelectClassicMenu( menu );
+										onClick={ async () => {
+											setIsUpdatingMenuRef( true );
+											await onSelectClassicMenu( menu );
+											setIsUpdatingMenuRef( false );
 											onClose();
 										} }
 										key={ menu.id }
 										aria-label={ sprintf(
 											createActionLabel,
 											label
 										) }
-										disabled={ isCreatingMenu }
+										disabled={
+											isUpdatingMenuRef ||
+											isResolvingNavigationMenus ||
+											! hasResolvedNavigationMenus
+										}
 									>
 										{ label }
 									</MenuItem>
@@ -177,12 +190,17 @@ function NavigationMenuSelector( {
 					{ canUserCreateNavigationMenu && (
 						<MenuGroup label={ __( 'Tools' ) }>
 							<MenuItem
-								disabled={ isCreatingMenu }
-								onClick={ () => {
+								onClick={ async () => {
+									setIsUpdatingMenuRef( true );
+									await onCreateNew();
+									setIsUpdatingMenuRef( false );
 									onClose();
-									onCreateNew();
-									setIsCreatingMenu( true );
 								} }
+								disabled={
+									isUpdatingMenuRef ||
+									isResolvingNavigationMenus ||
+									! hasResolvedNavigationMenus
+								}
 							>
 								{ __( 'Create new menu' ) }
 							</MenuItem>

diff --git a/packages/block-library/src/navigation/edit/test/navigation-menu-selector.js b/packages/block-library/src/navigation/edit/test/navigation-menu-selector.js
@@ -250,6 +250,7 @@ describe( 'NavigationMenuSelector', () => {
 				const user = userEvent.setup();
 				const handler = jest.fn();
 
+				// at the start we have the menus and we're not waiting on network
 				useNavigationMenu.mockReturnValue( {
 					navigationMenus: [],
 					hasResolvedNavigationMenus: true,
@@ -271,6 +272,18 @@ describe( 'NavigationMenuSelector', () => {
 					} )
 				);
 
+				// creating a menu is a network activity
+				// so we have to wait on it
+				useNavigationMenu.mockReturnValue( {
+					navigationMenus: [],
+					hasResolvedNavigationMenus: false,
+					isResolvingNavigationMenus: true,
+					canUserCreateNavigationMenu: true,
+					canSwitchNavigationMenu: true,
+				} );
+
+				rerender( <NavigationMenuSelector onCreateNew={ handler } /> );
+
 				// Re-open the dropdown (it's closed when the "Create menu" button is clicked).
 				await user.click( toggleButton );
 
@@ -284,6 +297,16 @@ describe( 'NavigationMenuSelector', () => {
 					} )
 				).toBeDisabled();
 
+				// once the menu is created
+				// no more network activity to wait on
+				useNavigationMenu.mockReturnValue( {
+					navigationMenus: [],
+					hasResolvedNavigationMenus: true,
+					isResolvingNavigationMenus: false,
+					canUserCreateNavigationMenu: true,
+					canSwitchNavigationMenu: true,
+				} );
+
 				// Simulate the menu being created and component being re-rendered.
 				rerender(
 					<NavigationMenuSelector
@@ -422,7 +445,7 @@ describe( 'NavigationMenuSelector', () => {
 				expect( menuItem ).toBeChecked();
 			} );
 
-			it( 'should call the handler when the navigation menu is selected and disable all options during the import/creation process', async () => {
+			it( 'should call the handler when the navigation menu is selected', async () => {
 				const user = userEvent.setup();
 
 				const handler = jest.fn();
@@ -434,7 +457,7 @@ describe( 'NavigationMenuSelector', () => {
 					canSwitchNavigationMenu: true,
 				} );
 
-				const { rerender } = render(
+				render(
 					<NavigationMenuSelector
 						onSelectNavigationMenu={ handler }
 					/>
@@ -455,42 +478,6 @@ describe( 'NavigationMenuSelector', () => {
 
 				//  Check the dropdown has been closed.
 				expect( screen.queryByRole( 'menu' ) ).not.toBeInTheDocument();
-
-				// Re-open the dropdown
-				await user.click( screen.getByRole( 'button' ) );
-
-				// Check the dropdown is again open and is in the "loading" state.
-				expect(
-					screen.getByRole( 'menu', {
-						name: /Loading/,
-					} )
-				).toBeInTheDocument();
-
-				// // Check all menu items are present but disabled.
-				screen.getAllByRole( 'menuitem' ).forEach( ( item ) => {
-					// // Check all menu items are present but disabled.
-					expect( item ).toBeDisabled();
-				} );
-
-				// // Simulate the menu being created and component being re-rendered.
-				rerender(
-					<NavigationMenuSelector
-						createNavigationMenuIsSuccess // classic menu import creates a Navigation menu.
-					/>
-				);
-
-				// Todo: fix bug where aria label is not updated.
-				// expect(
-				// 	screen.getByRole( 'menu', {
-				// 		name: `You are currently editing ${ navigationMenusFixture[ 0 ].title.rendered }`,
-				// 	} )
-				// ).toBeInTheDocument();
-
-				// Check all menu items are re-enabled.
-				screen.getAllByRole( 'menuitem' ).forEach( ( item ) => {
-					// // Check all menu items are present but disabled.
-					expect( item ).toBeEnabled();
-				} );
 			} );
 		} );
 
@@ -568,9 +555,14 @@ describe( 'NavigationMenuSelector', () => {
 
 			it( 'should call the handler when the classic menu item is selected and disable all options during the import/creation process', async () => {
 				const user = userEvent.setup();
-				const handler = jest.fn();
+				const handler = jest.fn( async () => {} );
 
+				// initially we have the menus, and we're not waiting on network
 				useNavigationMenu.mockReturnValue( {
+					navigationMenus: [],
+					isResolvingNavigationMenus: false,
+					hasResolvedNavigationMenus: true,
+					canSwitchNavigationMenu: true,
 					canUserCreateNavigationMenu: true,
 				} );
 
@@ -597,6 +589,23 @@ describe( 'NavigationMenuSelector', () => {
 				// Check the dropdown has been closed.
 				expect( screen.queryByRole( 'menu' ) ).not.toBeInTheDocument();
 
+				// since we're importing we are doing network activity
+				// so we have to wait on it
+				useNavigationMenu.mockReturnValue( {
+					navigationMenus: [],
+					isResolvingNavigationMenus: true,
+					hasResolvedNavigationMenus: false,
+					canUserCreateNavigationMenu: true,
+				} );
+
+				useNavigationEntities.mockReturnValue( {
+					menus: classicMenusFixture,
+				} );
+
+				rerender(
+					<NavigationMenuSelector onSelectClassicMenu={ handler } />
+				);
+
 				// // Re-open the dropdown (it's closed when the "Create menu" button is clicked).
 				await user.click( screen.getByRole( 'button' ) );
 
@@ -613,6 +622,19 @@ describe( 'NavigationMenuSelector', () => {
 					expect( item ).toBeDisabled();
 				} );
 
+				// once the menu is imported
+				// no more network activity to wait on
+				useNavigationMenu.mockReturnValue( {
+					navigationMenus: [],
+					isResolvingNavigationMenus: false,
+					hasResolvedNavigationMenus: true,
+					canUserCreateNavigationMenu: true,
+				} );
+
+				useNavigationEntities.mockReturnValue( {
+					menus: classicMenusFixture,
+				} );
+
 				// Simulate the menu being created and component being re-rendered.
 				rerender(
 					<NavigationMenuSelector