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.

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

Jump to bottom

Refactor away state in Nav menu selector #45464

Merged
merged 6 commits into from
Mar 2, 2023
Merged

Refactor away state in Nav menu selector #45464

Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
e84ff90
Refactor away set state in render and effect
getdave Feb 27, 2023
7f5f002
Pass correct deps to effect
getdave Feb 27, 2023
dea3e92
Pass correct deps to useMemo
getdave Feb 27, 2023
d51172f
Add comment for clarity
getdave Feb 27, 2023
90b6737
Add setters a dep to resolve lint
getdave Mar 2, 2023
4df1842
Fix comma lint
getdave Mar 2, 2023
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
62 changes: 37 additions & 25 deletions packages/block-library/src/navigation/edit/navigation-menu-selector.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,13 +11,22 @@ import { moreVertical } from '@wordpress/icons';
import { __, sprintf } from '@wordpress/i18n';
import { decodeEntities } from '@wordpress/html-entities';
import { useEffect, useMemo, useState } from '@wordpress/element';
import { useEntityProp } from '@wordpress/core-data';

/**
* Internal dependencies
*/
import useNavigationMenu from '../use-navigation-menu';
import useNavigationEntities from '../use-navigation-entities';

function buildMenuLabel( title, id ) {
const label =
decodeEntities( title?.rendered ) ||
/* translators: %s is the index of the menu in the list of menus. */
sprintf( __( '(no title %s)' ), id );
return label;
}

function NavigationMenuSelector( {
currentMenuId,
onSelectNavigationMenu,
Expand All @@ -30,7 +39,6 @@ function NavigationMenuSelector( {
/* translators: %s: The name of a menu. */
const createActionLabel = __( "Create from '%s'" );

const [ selectorLabel, setSelectorLabel ] = useState( '' );
const [ isCreatingMenu, setIsCreatingMenu ] = useState( false );

actionLabel = actionLabel || createActionLabel;
Expand All @@ -39,33 +47,31 @@ function NavigationMenuSelector( {

const {
navigationMenus,
isResolvingNavigationMenus,
hasResolvedNavigationMenus,
canUserCreateNavigationMenu,
canSwitchNavigationMenu,
} = useNavigationMenu();

const [ currentTitle ] = useEntityProp(
'postType',
'wp_navigation',
'title'
);

const menuChoices = useMemo( () => {
return (
navigationMenus?.map( ( { id, title }, index ) => {
const label =
decodeEntities( title?.rendered ) ||
/* translators: %s is the index of the menu in the list of menus. */
sprintf( __( '(no title %s)' ), index + 1 );

if ( id === currentMenuId && ! isCreatingMenu ) {
setSelectorLabel(
/* translators: %s is the name of a navigation menu. */
sprintf( __( 'You are currently editing %s' ), label )
);
}
const label = buildMenuLabel( title, index + 1 );

return {
value: id,
label,
ariaLabel: sprintf( actionLabel, label ),
};
} ) || []
);
}, [ currentMenuId, navigationMenus, actionLabel, isCreatingMenu ] );
}, [ navigationMenus, actionLabel ] );

const hasNavigationMenus = !! navigationMenus?.length;
const hasClassicMenus = !! classicMenus?.length;
Expand All @@ -77,20 +83,31 @@ function NavigationMenuSelector( {
const menuUnavailable =
hasResolvedNavigationMenus && currentMenuId === null;

useEffect( () => {
if ( ! hasResolvedNavigationMenus && ! canUserCreateNavigationMenu ) {
setSelectorLabel( __( 'Loading …' ) );
} else if ( noMenuSelected || noBlockMenus || menuUnavailable ) {
setSelectorLabel( __( 'Choose or create a Navigation menu' ) );
}
let selectorLabel = '';

if ( isCreatingMenu || isResolvingNavigationMenus ) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Made this loading state conditional on actually being in a resolving state rather than whether we have resolved or not.

selectorLabel = __( 'Loading …' );
} else if ( noMenuSelected || noBlockMenus || menuUnavailable ) {
getdave marked this conversation as resolved.
Show resolved Hide resolved
// Note: classic Menus may be available.
selectorLabel = __( 'Choose or create a Navigation menu' );
} else {
// Current Menu's title.
selectorLabel = currentTitle;
}

useEffect( () => {
if (
isCreatingMenu &&
( createNavigationMenuIsSuccess || createNavigationMenuIsError )
) {
setIsCreatingMenu( false );
getdave marked this conversation as resolved.
Show resolved Hide resolved
}
}, [ hasResolvedNavigationMenus, createNavigationMenuIsSuccess ] );
}, [
isCreatingMenu,
createNavigationMenuIsError,
getdave marked this conversation as resolved.
Show resolved Hide resolved
createNavigationMenuIsSuccess,
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we also add setIsCreatingMenu as a dependency?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

useState setters are guaranteed by React to display referential equality. I think this is because they derive from dispatch which is the same. I can double check that but I'm pretty sure it's correct.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we need to pass it as a dependency because we think it might change, only because we have a warning about undeclared dependencies.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I committed a change. Does that work?

getdave marked this conversation as resolved.
Show resolved Hide resolved
setIsCreatingMenu,
] );

const NavigationMenuSelectorDropdown = (
<DropdownMenu
Expand All @@ -105,7 +122,6 @@ function NavigationMenuSelector( {
<MenuItemsChoice
value={ currentMenuId }
onSelect={ ( menuId ) => {
setSelectorLabel( __( 'Loading …' ) );
setIsCreatingMenu( true );
onSelectNavigationMenu( menuId );
onClose();
Expand All @@ -122,9 +138,6 @@ function NavigationMenuSelector( {
return (
<MenuItem
onClick={ () => {
setSelectorLabel(
__( 'Loading …' )
);
setIsCreatingMenu( true );
onSelectClassicMenu( menu );
onClose();
Expand All @@ -151,7 +164,6 @@ function NavigationMenuSelector( {
onClose();
onCreateNew();
setIsCreatingMenu( true );
setSelectorLabel( __( 'Loading …' ) );
} }
>
{ __( 'Create new menu' ) }
Expand Down