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

Refactor Nav creation hooks and associated code #38824

Closed
wants to merge 6 commits into from
Closed

Refactor Nav creation hooks and associated code #38824

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
62234f5
Refactor hook to expose data
getdave Feb 15, 2022
b8bc7da
Refactor away classic menu creation code
getdave Feb 15, 2022
a046fd5
Call menu conversion callback with correct args
getdave Feb 15, 2022
a9e8980
Use identical hooks in placeholder
getdave Feb 15, 2022
fa71d7c
Remove callback
getdave Feb 15, 2022
8fec011
Rename exposed as run
getdave Feb 15, 2022
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
41 changes: 41 additions & 0 deletions packages/block-library/src/navigation/edit/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,8 @@ import UnsavedInnerBlocks from './unsaved-inner-blocks';
import NavigationMenuDeleteControl from './navigation-menu-delete-control';
import useNavigationNotice from './use-navigation-notice';
import OverlayMenuIcon from './overlay-menu-icon';
import useConvertClassicMenu from '../use-convert-classic-menu';
import useCreateNavigationMenu from './use-create-navigation-menu';

const EMPTY_ARRAY = [];

Expand Down Expand Up @@ -231,6 +233,16 @@ function Navigation( {
hasResolvedCanUserCreateNavigation,
} = useNavigationMenu( ref );

const createNavigationMenu = useCreateNavigationMenu( clientId );

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

const navRef = useRef();
const isDraftNavigationMenu = navigationMenu?.status === 'draft';

Expand Down Expand Up @@ -385,6 +397,27 @@ function Navigation( {
ref,
] );

useEffect( () => {
async function handleCreateNav() {
const navigationMenuPost = await createNavigationMenu(
classicMenuName,
classicMenuBlocks
);
setRef( navigationMenuPost?.id );
}
if (
hasResolvedClassicMenuConversion &&
classicMenuName &&
classicMenuBlocks?.length
Copy link
Contributor

Choose a reason for hiding this comment

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

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 😄 )

) {
handleCreateNav();
}
}, [
classicMenuBlocks,
classicMenuName,
hasResolvedClassicMenuConversion,
] );

const startWithEmptyMenu = useCallback( () => {
registry.batch( () => {
if ( navigationArea ) {
Expand Down Expand Up @@ -495,6 +528,13 @@ function Navigation( {
setRef( id );
onClose();
} }
onSelectClassicMenu={ ( menu ) => {
onClose();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Need to be a little careful about focus management here.

convertClassicMenuToBlocks(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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).

menu?.id,
menu?.name
);
} }
onCreateNew={ startWithEmptyMenu }
/* translators: %s: The name of a menu. */
actionLabel={ __( "Switch to '%s'" ) }
Expand Down Expand Up @@ -676,6 +716,7 @@ function Navigation( {
/>
) }
{ ! hasResolvedCanUserCreateNavigation ||
isResolvingClassicMenuConversion ||
( ! isEntityAvailable && ! isPlaceholderShown && (
<PlaceholderPreview isLoading />
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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).

) ) }
Expand Down
30 changes: 2 additions & 28 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,12 +11,10 @@ import { addQueryArgs } from '@wordpress/url';
*/
import useNavigationMenu from '../use-navigation-menu';
import useNavigationEntities from '../use-navigation-entities';
import useConvertClassicMenu from '../use-convert-classic-menu';
import useCreateNavigationMenu from './use-create-navigation-menu';

export default function NavigationMenuSelector( {
clientId,
onSelect,
onSelectClassicMenu,
onCreateNew,
showManageActions = false,
actionLabel,
Expand All @@ -35,27 +33,6 @@ export default function NavigationMenuSelector( {
canSwitchNavigationMenu,
} = useNavigationMenu();

const createNavigationMenu = useCreateNavigationMenu( clientId );

const onFinishMenuCreation = async (
blocks,
navigationMenuTitle = null
) => {
if ( ! canUserCreateNavigationMenu ) {
return;
}

const navigationMenu = await createNavigationMenu(
navigationMenuTitle,
blocks
);
onSelect( navigationMenu );
};

const convertClassicMenuToBlocks = useConvertClassicMenu(
onFinishMenuCreation
);

const hasNavigationMenus = !! navigationMenus?.length;
const hasClassicMenus = !! classicMenus?.length;
const showNavigationMenus = !! canSwitchNavigationMenu;
Expand Down Expand Up @@ -97,10 +74,7 @@ export default function NavigationMenuSelector( {
return (
<MenuItem
onClick={ () => {
convertClassicMenuToBlocks(
menu.id,
menu.name
);
onSelectClassicMenu( menu );
} }
key={ menu.id }
aria-label={ sprintf(
Expand Down
47 changes: 45 additions & 2 deletions packages/block-library/src/navigation/edit/placeholder/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
import { Placeholder, Button, DropdownMenu } from '@wordpress/components';
import { __ } from '@wordpress/i18n';
import { navigation, Icon } from '@wordpress/icons';
import { useEffect } from '@wordpress/element';

/**
* Internal dependencies
Expand All @@ -14,6 +15,7 @@ import PlaceholderPreview from './placeholder-preview';
import useNavigationMenu from '../../use-navigation-menu';
import useCreateNavigationMenu from '../use-create-navigation-menu';
import NavigationMenuSelector from '../navigation-menu-selector';
import useConvertClassicMenu from '../../use-convert-classic-menu';

export default function NavigationPlaceholder( {
clientId,
Expand Down Expand Up @@ -45,7 +47,39 @@ export default function NavigationPlaceholder( {
hasMenus,
} = useNavigationEntities();

const isStillLoading = isResolvingPages || isResolvingMenus;
const {
dispatch: convertClassicMenuToBlocks,
blocks: classicMenuBlocks,
name: classicMenuName,
isResolving: isResolvingClassicMenuConversion,
hasResolved: hasResolvedClassicMenuConversion,
} = useConvertClassicMenu( () => {} );

useEffect( () => {
async function handleCreateNav() {
const navigationMenuPost = await createNavigationMenu(
classicMenuName,
classicMenuBlocks
);
onFinish( navigationMenuPost, classicMenuBlocks );
}
if (
hasResolvedClassicMenuConversion &&
classicMenuName &&
classicMenuBlocks?.length
) {
handleCreateNav();
}
}, [
classicMenuBlocks,
classicMenuName,
hasResolvedClassicMenuConversion,
] );

const isStillLoading =
Copy link
Contributor

Choose a reason for hiding this comment

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

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 =

isResolvingPages ||
isResolvingMenus ||
isResolvingClassicMenuConversion;

const onCreateEmptyMenu = () => {
onFinishMenuCreation( [] );
Expand Down Expand Up @@ -89,10 +123,19 @@ export default function NavigationPlaceholder( {
} }
popoverProps={ { isAlternate: true } }
>
{ () => (
{ ( { onClose } ) => (
<NavigationMenuSelector
clientId={ clientId }
onSelect={ onFinish }
onSelectClassicMenu={ (
menu
) => {
onClose();
convertClassicMenuToBlocks(
menu?.id,
menu?.name
);
} }
/>
) }
</DropdownMenu>
Expand Down
24 changes: 18 additions & 6 deletions packages/block-library/src/navigation/use-convert-classic-menu.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,15 +16,19 @@ export default function useConvertClassicMenu( onFinish ) {
setIsAwaitingMenuItemResolution,
] = useState( false );
const [ menuName, setMenuName ] = useState( '' );
const [ blocks, setBlocks ] = useState( [] );

const { menuItems, hasResolvedMenuItems } = useNavigationEntities(
selectedMenu
);
const {
menuItems,
hasResolvedMenuItems,
isResolvingMenus,
} = useNavigationEntities( selectedMenu );

const createFromMenu = useCallback(
( name ) => {
const { innerBlocks: blocks } = menuItemsToBlocks( menuItems );
onFinish( blocks, name );
const { innerBlocks: _blocks } = menuItemsToBlocks( menuItems );
setBlocks( _blocks );
onFinish( _blocks, name );
},
[ menuItems, menuItemsToBlocks, onFinish ]
);
Expand All @@ -38,7 +42,7 @@ export default function useConvertClassicMenu( onFinish ) {
}
}, [ isAwaitingMenuItemResolution, hasResolvedMenuItems, menuName ] );

return useCallback(
const dispatch = useCallback(
( id, name ) => {
setSelectedMenu( id );

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

return {
dispatch,
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 don't love dispatch here. We could use run or similar?

Copy link
Contributor

Choose a reason for hiding this comment

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

convert?

blocks,
name: menuName,
isResolving: isResolvingMenus,
hasResolved: hasResolvedMenuItems,
};
}