Skip to content
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.

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

Fix create menu after menu switch #59630

Merged
merged 9 commits into from
Mar 11, 2024
35 changes: 23 additions & 12 deletions packages/block-library/src/navigation/edit/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -154,7 +154,10 @@ function Navigation( {
} = useCreateNavigationMenu( clientId );

const createUntitledEmptyNavigationMenu = () => {
createNavigationMenu( '' );
return new Promise( async ( resolve ) => {
await createNavigationMenu( '' );
resolve();
} );
draganescu marked this conversation as resolved.
Show resolved Hide resolved
};

const {
Expand Down Expand Up @@ -342,20 +345,28 @@ function Navigation( {
const [ detectedOverlayColor, setDetectedOverlayColor ] = useState();

const onSelectClassicMenu = async ( classicMenu ) => {
const navMenu = await convertClassicMenu(
classicMenu.id,
classicMenu.name,
'draft'
);
if ( navMenu ) {
handleUpdateMenu( navMenu.id, {
focusNavigationBlock: true,
} );
}
return new Promise( async ( resolve ) => {
draganescu marked this conversation as resolved.
Show resolved Hide resolved
const navMenu = await convertClassicMenu(
classicMenu.id,
classicMenu.name,
'draft'
);
if ( navMenu ) {
handleUpdateMenu( navMenu.id, {
focusNavigationBlock: true,
} );
}
resolve();
} );
};

const onSelectNavigationMenu = ( menuId ) => {
handleUpdateMenu( menuId );
return new Promise( ( resolve ) => {
handleUpdateMenu( menuId, {
focusNavigationBlock: true,
} );
resolve();
} );
getdave marked this conversation as resolved.
Show resolved Hide resolved
};

useEffect( () => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down Expand Up @@ -99,7 +99,7 @@ function NavigationMenuSelector( {

let selectorLabel = '';

if ( isCreatingMenu || isResolvingNavigationMenus ) {
if ( isResolvingNavigationMenus ) {
selectorLabel = __( 'Loading…' );
} else if ( noMenuSelected || noBlockMenus || menuUnavailable ) {
// Note: classic Menus may be available.
Expand All @@ -111,17 +111,17 @@ function NavigationMenuSelector( {

useEffect( () => {
if (
isCreatingMenu &&
isUpdatingMenuRef &&
( createNavigationMenuIsSuccess || createNavigationMenuIsError )
) {
setIsCreatingMenu( false );
setIsUpdatingMenuRef( false );
}
}, [
hasResolvedNavigationMenus,
createNavigationMenuIsSuccess,
canUserCreateNavigationMenu,
createNavigationMenuIsError,
isCreatingMenu,
isUpdatingMenuRef,
menuUnavailable,
noBlockMenus,
noMenuSelected,
Comment on lines 130 to 137
Copy link
Contributor

Choose a reason for hiding this comment

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

We should be able to remove all the unused deps from this effect.

I propose we do this in a followup.

Expand All @@ -140,12 +140,19 @@ function NavigationMenuSelector( {
<MenuItemsChoice
value={ currentMenuId }
onSelect={ ( menuId ) => {
setIsCreatingMenu( true );
onSelectNavigationMenu( menuId );
onClose();
setIsUpdatingMenuRef( true );
onSelectNavigationMenu( menuId ).then(
() => {
setIsUpdatingMenuRef( false );
onClose();
}
);
} }
choices={ menuChoices }
disabled={ isCreatingMenu }
disabled={
isUpdatingMenuRef ||
! hasResolvedNavigationMenus
}
/>
</MenuGroup>
) }
Expand All @@ -156,16 +163,25 @@ function NavigationMenuSelector( {
return (
<MenuItem
onClick={ () => {
setIsCreatingMenu( true );
onSelectClassicMenu( menu );
onClose();
setIsUpdatingMenuRef( true );
onSelectClassicMenu( menu ).then(
() => {
setIsUpdatingMenuRef(
false
);
onClose();
}
);
} }
key={ menu.id }
aria-label={ sprintf(
createActionLabel,
label
) }
disabled={ isCreatingMenu }
disabled={
isUpdatingMenuRef ||
! hasResolvedNavigationMenus
}
>
{ label }
</MenuItem>
Expand All @@ -177,11 +193,16 @@ function NavigationMenuSelector( {
{ canUserCreateNavigationMenu && (
<MenuGroup label={ __( 'Tools' ) }>
<MenuItem
disabled={ isCreatingMenu }
disabled={
isUpdatingMenuRef ||
! hasResolvedNavigationMenus
}
onClick={ () => {
onClose();
onCreateNew();
setIsCreatingMenu( true );
setIsUpdatingMenuRef( true );
onCreateNew().then( () => {
setIsUpdatingMenuRef( false );
draganescu marked this conversation as resolved.
Show resolved Hide resolved
onClose();
} );
} }
>
{ __( 'Create new menu' ) }
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@
},
status: 'publish',
};
const navigationMenu2 = {

Check failure on line 37 in packages/block-library/src/navigation/edit/test/navigation-menu-selector.js

View workflow job for this annotation

GitHub Actions / JavaScript (Node.js 21)

TestingLibraryElementError: Unable to find an accessible element with the role "menu" and name `/Loading/` Here are the accessible roles: menu: Name "Choose or create a Navigation menu": <div aria-label="Choose or create a Navigation menu" aria-orientation="vertical" class="components-dropdown-menu__menu" role="menu" /> -------------------------------------------------- group: Name "Menus": <div aria-labelledby="components-menu-group-label-12" role="group" /> Name "Tools": <div aria-labelledby="components-menu-group-label-13" role="group" /> -------------------------------------------------- menuitemradio: Name "Menu 1": <button aria-checked="false" class="components-button components-menu-item__button components-menu-items-choice" role="menuitemradio" type="button" /> Name "Menu 2": <button aria-checked="false" class="components-button components-menu-item__button components-menu-items-choice" role="menuitemradio" type="button" /> Name "Menu 3": <button aria-checked="false" class="components-button components-menu-item__button components-menu-items-choice" role="menuitemradio" type="button" /> -------------------------------------------------- menuitem: Name "Create new menu": <button class="components-button components-menu-item__button" role="menuitem" type="button" /> --------------------------------------------------button: Name "Choose or create a Navigation menu": <button aria-expanded="true" aria-haspopup="true" aria-label="Choose or create a Navigation menu" class="components-button components-dropdown-menu__toggle is-opened is-small has-icon" type="button" /> -------------------------------------------------- Ignored nodes: comments, script, style <body> <p class="a11y-speak-intro-text" hidden="hidden" id="a11y-speak-intro-text" style="position: absolute;margin: -1px;padding: 0;height: 1px;width: 1px;overflow: hidden;clip: rect(1px, 1px, 1px, 1px);-webkit-clip-path: inset(50%);clip-path: inset(50%);border: 0;word-wrap: normal !important;" > Notifications </p> <div aria-atomic="true" aria-live="assertive" aria-relevant="additions text" class="a11y-speak-region" id="a11y-speak-assertive" style="position: absolute;margin: -1px;padding: 0;height: 1px;width: 1px;overflow: hidden;clip: rect(1px, 1px, 1px, 1px);-webkit-clip-path: inset(50%);clip-path: inset(50%);border: 0;word-wrap: normal !important;" /> <div aria-atomic="true" aria-live="polite" aria-relevant="additions text" class="a11y-speak-region" id="a11y-speak-polite" style="position: absolute;margin: -1px;padding: 0;height: 1px;width: 1px;overflow: hidden;clip: rect(1px, 1px, 1px, 1px);-webkit-clip-path: inset(50%);clip-path: inset(50%);border: 0;word-wrap: normal !important;" />

Check failure on line 37 in packages/block-library/src/navigation/edit/test/navigation-menu-selector.js

View workflow job for this annotation

GitHub Actions / JavaScript (Node.js 21)

TestingLibraryElementError: Unable to find an accessible element with the role "menuitem" Here are the accessible roles: menu: Name "Choose or create a Navigation menu": <div aria-label="Choose or create a Navigation menu" aria-orientation="vertical" class="components-dropdown-menu__menu" role="menu" /> --------------------------------------------------button: Name "Choose or create a Navigation menu": <button aria-expanded="true" aria-haspopup="true" aria-label="Choose or create a Navigation menu" class="components-button components-dropdown-menu__toggle is-opened is-small has-icon" type="button" /> -------------------------------------------------- Ignored nodes: comments, script, style <body> <p class="a11y-speak-intro-text" hidden="hidden" id="a11y-speak-intro-text" style="position: absolute;margin: -1px;padding: 0;height: 1px;width: 1px;overflow: hidden;clip: rect(1px, 1px, 1px, 1px);-webkit-clip-path: inset(50%);clip-path: inset(50%);border: 0;word-wrap: normal !important;" > Notifications </p> <div aria-atomic="true" aria-live="assertive" aria-relevant="additions text" class="a11y-speak-region" id="a11y-speak-assertive" style="position: absolute;margin: -1px;padding: 0;height: 1px;width: 1px;overflow: hidden;clip: rect(1px, 1px, 1px, 1px);-webkit-clip-path: inset(50%);clip-path: inset(50%);border: 0;word-wrap: normal !important;" /> <div aria-atomic="true" aria-live="polite" aria-relevant="additions text" class="a11y-speak-region" id="a11y-speak-polite" style="position: absolute;margin: -1px;padding: 0;height: 1px;width: 1px;overflow: hidden;clip: rect(1px, 1px, 1px, 1px);-webkit-clip-path: inset(50%);clip-path: inset(50%);border: 0;word-wrap: normal !important;" /> <div class="components-popover__fallback-container" > <div class="components-popover components-popover components-dropdown__content components-dropdown-menu__popover is-positioned" data-wp-c16t="true" data-wp-component="Popover" style="position: absolute; top: 0px; left: 0px; opacity: 1; transform: translateX(0px) translateY(13px) translateY(0px) scale(1) translateZ(0); transform-origin: 0% 0% 0;" tabindex="-1" > <div class="components-popover__content" style="max-height: -13px; overflow: auto;" > <div aria-label="Choose or create a Navigation menu" aria-orientation="vertical" class="components-dropdown-menu__menu" role="menu" /> </div> </div> </div> <div> <div class="components-dropdown components-dropdown-menu" tabindex="-1" > <button aria-expanded="true" aria-haspopup="true" aria-label="Choose or create a Navigation menu" class="components-button components-dropdown-menu__toggle is-opened is-small h

Check failure on line 37 in packages/block-library/src/navigation/edit/test/navigation-menu-selector.js

View workflow job for this annotation

GitHub Actions / JavaScript (Node.js 20)

TestingLibraryElementError: Unable to find an accessible element with the role "menu" and name `/Loading/` Here are the accessible roles: menu: Name "Choose or create a Navigation menu": <div aria-label="Choose or create a Navigation menu" aria-orientation="vertical" class="components-dropdown-menu__menu" role="menu" /> -------------------------------------------------- group: Name "Menus": <div aria-labelledby="components-menu-group-label-12" role="group" /> Name "Tools": <div aria-labelledby="components-menu-group-label-13" role="group" /> -------------------------------------------------- menuitemradio: Name "Menu 1": <button aria-checked="false" class="components-button components-menu-item__button components-menu-items-choice" role="menuitemradio" type="button" /> Name "Menu 2": <button aria-checked="false" class="components-button components-menu-item__button components-menu-items-choice" role="menuitemradio" type="button" /> Name "Menu 3": <button aria-checked="false" class="components-button components-menu-item__button components-menu-items-choice" role="menuitemradio" type="button" /> -------------------------------------------------- menuitem: Name "Create new menu": <button class="components-button components-menu-item__button" role="menuitem" type="button" /> --------------------------------------------------button: Name "Choose or create a Navigation menu": <button aria-expanded="true" aria-haspopup="true" aria-label="Choose or create a Navigation menu" class="components-button components-dropdown-menu__toggle is-opened is-small has-icon" type="button" /> -------------------------------------------------- Ignored nodes: comments, script, style <body> <p class="a11y-speak-intro-text" hidden="hidden" id="a11y-speak-intro-text" style="position: absolute;margin: -1px;padding: 0;height: 1px;width: 1px;overflow: hidden;clip: rect(1px, 1px, 1px, 1px);-webkit-clip-path: inset(50%);clip-path: inset(50%);border: 0;word-wrap: normal !important;" > Notifications </p> <div aria-atomic="true" aria-live="assertive" aria-relevant="additions text" class="a11y-speak-region" id="a11y-speak-assertive" style="position: absolute;margin: -1px;padding: 0;height: 1px;width: 1px;overflow: hidden;clip: rect(1px, 1px, 1px, 1px);-webkit-clip-path: inset(50%);clip-path: inset(50%);border: 0;word-wrap: normal !important;" /> <div aria-atomic="true" aria-live="polite" aria-relevant="additions text" class="a11y-speak-region" id="a11y-speak-polite" style="position: absolute;margin: -1px;padding: 0;height: 1px;width: 1px;overflow: hidden;clip: rect(1px, 1px, 1px, 1px);-webkit-clip-path: inset(50%);clip-path: inset(50%);border: 0;word-wrap: normal !important;" />

Check failure on line 37 in packages/block-library/src/navigation/edit/test/navigation-menu-selector.js

View workflow job for this annotation

GitHub Actions / JavaScript (Node.js 20)

TestingLibraryElementError: Unable to find an accessible element with the role "menuitem" Here are the accessible roles: menu: Name "Choose or create a Navigation menu": <div aria-label="Choose or create a Navigation menu" aria-orientation="vertical" class="components-dropdown-menu__menu" role="menu" /> --------------------------------------------------button: Name "Choose or create a Navigation menu": <button aria-expanded="true" aria-haspopup="true" aria-label="Choose or create a Navigation menu" class="components-button components-dropdown-menu__toggle is-opened is-small has-icon" type="button" /> -------------------------------------------------- Ignored nodes: comments, script, style <body> <p class="a11y-speak-intro-text" hidden="hidden" id="a11y-speak-intro-text" style="position: absolute;margin: -1px;padding: 0;height: 1px;width: 1px;overflow: hidden;clip: rect(1px, 1px, 1px, 1px);-webkit-clip-path: inset(50%);clip-path: inset(50%);border: 0;word-wrap: normal !important;" > Notifications </p> <div aria-atomic="true" aria-live="assertive" aria-relevant="additions text" class="a11y-speak-region" id="a11y-speak-assertive" style="position: absolute;margin: -1px;padding: 0;height: 1px;width: 1px;overflow: hidden;clip: rect(1px, 1px, 1px, 1px);-webkit-clip-path: inset(50%);clip-path: inset(50%);border: 0;word-wrap: normal !important;" /> <div aria-atomic="true" aria-live="polite" aria-relevant="additions text" class="a11y-speak-region" id="a11y-speak-polite" style="position: absolute;margin: -1px;padding: 0;height: 1px;width: 1px;overflow: hidden;clip: rect(1px, 1px, 1px, 1px);-webkit-clip-path: inset(50%);clip-path: inset(50%);border: 0;word-wrap: normal !important;" /> <div class="components-popover__fallback-container" > <div class="components-popover components-popover components-dropdown__content components-dropdown-menu__popover is-positioned" data-wp-c16t="true" data-wp-component="Popover" style="position: absolute; top: 0px; left: 0px; opacity: 1; transform: translateX(0px) translateY(13px) translateY(0px) scale(1) translateZ(0); transform-origin: 0% 0% 0;" tabindex="-1" > <div class="components-popover__content" style="max-height: -13px; overflow: auto;" > <div aria-label="Choose or create a Navigation menu" aria-orientation="vertical" class="components-dropdown-menu__menu" role="menu" /> </div> </div> </div> <div> <div class="components-dropdown components-dropdown-menu" tabindex="-1" > <button aria-expanded="true" aria-haspopup="true" aria-label="Choose or create a Navigation menu" class="components-button components-dropdown-menu__toggle is-opened is-small h
id: 2,
title: {
rendered: 'Menu 2',
Expand Down Expand Up @@ -221,7 +221,12 @@

it( 'should call handler callback and close popover when create menu button is clicked', async () => {
const user = userEvent.setup();
const handler = jest.fn();
const handler = jest.fn(
() =>
new Promise( ( resolve ) => {
resolve();
} )
);

useNavigationMenu.mockReturnValue( {
navigationMenus: [],
Expand All @@ -248,7 +253,12 @@

it( 'should handle disabled state of the create menu button during the creation process', async () => {
const user = userEvent.setup();
const handler = jest.fn();
const handler = jest.fn(
() =>
new Promise( ( resolve ) => {
resolve();
} )
);

useNavigationMenu.mockReturnValue( {
navigationMenus: [],
Expand Down Expand Up @@ -425,7 +435,12 @@
it( 'should call the handler when the navigation menu is selected and disable all options during the import/creation process', async () => {
const user = userEvent.setup();

const handler = jest.fn();
const handler = jest.fn(
() =>
new Promise( ( resolve ) => {
resolve();
} )
);

useNavigationMenu.mockReturnValue( {
navigationMenus: navigationMenusFixture,
Expand Down Expand Up @@ -568,16 +583,23 @@

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();

useNavigationMenu.mockReturnValue( {
canUserCreateNavigationMenu: true,
} );
const handler = jest.fn(
() =>
new Promise( ( resolve ) => {
resolve();
} )
);

useNavigationEntities.mockReturnValue( {
menus: classicMenusFixture,
} );

useNavigationMenu.mockReturnValue( {
canUserCreateNavigationMenu: true,
isResolvingNavigationMenus: false,
hasResolvedNavigationMenus: true,
} );

const { rerender } = render(
<NavigationMenuSelector onSelectClassicMenu={ handler } />
);
Expand All @@ -594,6 +616,16 @@

expect( handler ).toHaveBeenCalled();

useNavigationMenu.mockReturnValue( {
canUserCreateNavigationMenu: true,
isResolvingNavigationMenus: true,
hasResolvedNavigationMenus: false,
} );

rerender(
<NavigationMenuSelector onSelectClassicMenu={ handler } />
);

// Check the dropdown has been closed.
expect( screen.queryByRole( 'menu' ) ).not.toBeInTheDocument();

Expand All @@ -613,6 +645,11 @@
expect( item ).toBeDisabled();
} );

useNavigationMenu.mockReturnValue( {
isResolvingNavigationMenus: false,
hasResolvedNavigationMenus: true,
} );

// Simulate the menu being created and component being re-rendered.
rerender(
<NavigationMenuSelector
Expand Down