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

Simplfy handling of save of Nav block uncontrolled inner blocks #45517

Merged
merged 5 commits into from
Nov 9, 2022
Merged

Simplfy handling of save of Nav block uncontrolled inner blocks #45517

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
c55d2b8
Simplfy handling of save of inner blocks
getdave Nov 9, 2022
f4fa98a
Remove unused dep from effect
getdave Nov 8, 2022
b582445
Memoize effect dep to avoid retriggering
getdave Nov 8, 2022
98da28c
Correctly memoize and sort all fallback menus
getdave Nov 9, 2022
b0aecfa
Simplify fallbacks compute function
getdave Nov 9, 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
53 changes: 25 additions & 28 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 @@ -6,7 +6,13 @@ import classnames from 'classnames';
/**
* WordPress dependencies
*/
import { useState, useEffect, useRef, Platform } from '@wordpress/element';
import {
useState,
useEffect,
useRef,
Platform,
useMemo,
} from '@wordpress/element';
import { addQueryArgs } from '@wordpress/url';
import {
__experimentalOffCanvasEditor as OffCanvasEditor,
Expand Down Expand Up @@ -197,9 +203,6 @@ function Navigation( {
__unstableMarkNextChangeAsNotPersistent,
} = useDispatch( blockEditorStore );

const [ hasSavedUnsavedInnerBlocks, setHasSavedUnsavedInnerBlocks ] =
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 shouldn't need this state in the parent.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, agree I don't think it's needed. I think it was only there to support the previous behavior where a menu would be created whenever UnsavedInnerBlocks mounted. From memory there where issues where UnsavedInnerBlocks would be unmounted and remounted and could cause duplicate menus to be created—the parent block needed a way to guard the menu creation process.

Now this only happens when a user selects/modifies the inner blocks, and I don't think as much guarding is needed. 👍

I think it's worth giving this a lot of manual testing to be sure, as it's hard to reason about some aspects of the nav block code and also core data resolution.

useState( false );

const [ isResponsiveMenuOpen, setResponsiveMenuVisibility ] =
useState( false );

Expand Down Expand Up @@ -233,8 +236,16 @@ function Navigation( {
classicMenuConversionStatus === CLASSIC_MENU_CONVERSION_PENDING;

// Only autofallback to published menus.
const fallbackNavigationMenus = navigationMenus?.filter(
( menu ) => menu.status === 'publish'
const fallbackNavigationMenus = useMemo(
() =>
navigationMenus
?.filter( ( menu ) => menu.status === 'publish' )
?.sort( ( menuA, menuB ) => {
const menuADate = new Date( menuA.date );
const menuBDate = new Date( menuB.date );
return menuADate.getTime() < menuBDate.getTime();
} ),
[ navigationMenus ]
);

// Attempt to retrieve and prioritize any existing navigation menu unless:
Expand All @@ -254,12 +265,6 @@ function Navigation( {
return;
}

fallbackNavigationMenus.sort( ( menuA, menuB ) => {
const menuADate = new Date( menuA.date );
const menuBDate = new Date( menuB.date );
return menuADate.getTime() < menuBDate.getTime();
} );

/**
* This fallback displays (both in editor and on front)
* a list of pages only if no menu (user assigned or
Expand All @@ -269,7 +274,12 @@ function Navigation( {
*/
__unstableMarkNextChangeAsNotPersistent();
setRef( fallbackNavigationMenus[ 0 ].id );
}, [ navigationMenus ] );
}, [
ref,
isCreatingNavigationMenu,
fallbackNavigationMenus,
getdave marked this conversation as resolved.
Show resolved Hide resolved
hasUncontrolledInnerBlocks,
] );

useEffect( () => {
if (
Expand Down Expand Up @@ -680,7 +690,7 @@ function Navigation( {
/>
);

if ( hasUnsavedBlocks ) {
if ( hasUnsavedBlocks && ! isCreatingNavigationMenu ) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Addition of ! isCreatingNavigationMenu is important here. We shouldn't render the uncontrolled inner blocks if a menu is being created. That way we can defer all the UI logic for a "loading" or "saving" state to the main edit.js component.

return (
<TagName { ...blockProps }>
<InspectorControls>
Expand Down Expand Up @@ -744,24 +754,11 @@ function Navigation( {
overlayTextColor={ overlayTextColor }
>
<UnsavedInnerBlocks
createNavigationMenu={ createNavigationMenu }
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here's where we pass down the primary creation function.

blocks={ uncontrolledInnerBlocks }
clientId={ clientId }
templateLock={ templateLock }
navigationMenus={ navigationMenus }
hasSelection={ isSelected || isInnerBlockSelected }
hasSavedUnsavedInnerBlocks={
hasSavedUnsavedInnerBlocks
}
onSave={ ( post ) => {
// Set some state used as a guard to prevent the creation of multiple posts.
setHasSavedUnsavedInnerBlocks( true );
// Switch to using the wp_navigation entity.
setRef( post.id );

showNavigationMenuStatusNotice(
__( `New Navigation Menu created.` )
);
} }
Comment on lines -755 to -764
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is now all defer to the centralised logic within the edit component which handles creation of menus. Saves maintaining another set of "loading" logic.

/>
</ResponsiveWrapper>
</TagName>
Expand Down
34 changes: 5 additions & 29 deletions packages/block-library/src/navigation/edit/unsaved-inner-blocks.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
* WordPress dependencies
*/
import { useInnerBlocksProps } from '@wordpress/block-editor';
import { Disabled, Spinner } from '@wordpress/components';
import { Disabled } from '@wordpress/components';
import { store as coreStore } from '@wordpress/core-data';
import { useSelect } from '@wordpress/data';
import { useContext, useEffect, useRef, useMemo } from '@wordpress/element';
Expand All @@ -11,7 +11,6 @@ import { useContext, useEffect, useRef, useMemo } from '@wordpress/element';
* Internal dependencies
*/
import useNavigationMenu from '../use-navigation-menu';
import useCreateNavigationMenu from './use-create-navigation-menu';

const EMPTY_OBJECT = {};
const DRAFT_MENU_PARAMS = [
Expand All @@ -38,9 +37,8 @@ const ALLOWED_BLOCKS = [

export default function UnsavedInnerBlocks( {
blocks,
clientId,
hasSavedUnsavedInnerBlocks,
onSave,
createNavigationMenu,

hasSelection,
} ) {
const originalBlocks = useRef();
Expand Down Expand Up @@ -75,7 +73,6 @@ export default function UnsavedInnerBlocks( {
// The block will be disabled in a block preview, use this as a way of
// avoiding the side-effects of this component for block previews.
const isDisabled = useContext( Disabled.Context );
const savingLock = useRef( false );

const innerBlocksProps = useInnerBlocksProps(
{
Expand Down Expand Up @@ -121,9 +118,6 @@ export default function UnsavedInnerBlocks( {

const { hasResolvedNavigationMenus, navigationMenus } = useNavigationMenu();

const { create: createNavigationMenu } =
useCreateNavigationMenu( clientId );

// Automatically save the uncontrolled blocks.
useEffect( () => {
// The block will be disabled when used in a BlockPreview.
Expand All @@ -140,9 +134,7 @@ export default function UnsavedInnerBlocks( {
// which is an indication they want to start editing.
if (
isDisabled ||
hasSavedUnsavedInnerBlocks ||
isSaving ||
savingLock.current ||
! hasResolvedDraftNavigationMenus ||
! hasResolvedNavigationMenus ||
! hasSelection ||
Expand All @@ -151,11 +143,7 @@ export default function UnsavedInnerBlocks( {
return;
}

savingLock.current = true;
createNavigationMenu( null, blocks ).then( ( menu ) => {
onSave( menu );
savingLock.current = false;
} );
createNavigationMenu( null, blocks );
}, [
isDisabled,
isSaving,
Expand All @@ -170,17 +158,5 @@ export default function UnsavedInnerBlocks( {

const Wrapper = isSaving ? Disabled : 'div';

return (
<>
{ isSaving ? (
<Spinner
className={
'wp-block-navigation__uncontrolled-inner-blocks-loading-indicator'
}
/>
) : (
<Wrapper { ...innerBlocksProps } />
) }
Comment on lines -175 to -183
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 no longer require this logic as the component will not be rendered when creating a menu.

</>
);
return <Wrapper { ...innerBlocksProps } />;
}