Ensure the overlay menu works when inserting navigation block pattern…

…s and that inner blocks are retained (#37358) * Show overlay menu for unsaved inner blocks too * Fix uncontrolled inner blocks being deleted * Add e2e test for converting markup inner blocks to a navigation post * Change useNavigationMenu back to trunk * Re-enable single entity test * Attempt to sync block editor store with controlled state Addresses #37358 (comment) * Add e2e test for lingering uncontrolled blocks issue Co-authored-by: Dave Smith <getdavemail@gmail.com>
WordPress · Dec 17, 2021 · 00bf8f6 · 00bf8f6
1 parent dddcb17
commit 00bf8f6
Show file tree

Hide file tree

Showing 4 changed files with 244 additions and 141 deletions.
diff --git a/packages/block-library/src/navigation/edit/index.js b/packages/block-library/src/navigation/edit/index.js
@@ -55,6 +55,8 @@ import NavigationMenuNameControl from './navigation-menu-name-control';
 import UnsavedInnerBlocks from './unsaved-inner-blocks';
 import NavigationMenuDeleteControl from './navigation-menu-delete-control';
 
+const EMPTY_ARRAY = [];
+
 function getComputedStyle( node ) {
 	return node.ownerDocument.defaultView.getComputedStyle( node );
 }
@@ -146,25 +148,44 @@ function Navigation( {
 		`navigationMenu/${ ref }`
 	);
 
-	const { innerBlocks, isInnerBlockSelected, hasSubmenus } = useSelect(
+	const {
+		hasUncontrolledInnerBlocks,
+		uncontrolledInnerBlocks,
+		controlledInnerBlocks,
+		isInnerBlockSelected,
+		hasSubmenus,
+	} = useSelect(
 		( select ) => {
-			const { getBlocks, hasSelectedInnerBlock } = select(
+			const { getBlock, getBlocks, hasSelectedInnerBlock } = select(
 				blockEditorStore
 			);
-			const blocks = getBlocks( clientId );
-			const firstSubmenu = !! blocks.find(
-				( block ) => block.name === 'core/navigation-submenu'
-			);
+
+			// This relies on the fact that `getBlock` won't return controlled
+			// inner blocks, while `getBlocks` does. It might be more stable to
+			// introduce a selector like `getUncontrolledInnerBlocks`, just in
+			// case `getBlock` is fixed.
+			const _uncontrolledInnerBlocks = getBlock( clientId ).innerBlocks;
+			const _hasUncontrolledInnerBlocks =
+				_uncontrolledInnerBlocks?.length;
+			const _controlledInnerBlocks = _hasUncontrolledInnerBlocks
+				? EMPTY_ARRAY
+				: getBlocks( clientId );
+			const innerBlocks = _hasUncontrolledInnerBlocks
+				? _uncontrolledInnerBlocks
+				: _controlledInnerBlocks;
 
 			return {
-				hasSubmenus: firstSubmenu,
-				innerBlocks: blocks,
+				hasSubmenus: !! innerBlocks.find(
+					( block ) => block.name === 'core/navigation-submenu'
+				),
+				hasUncontrolledInnerBlocks: _hasUncontrolledInnerBlocks,
+				uncontrolledInnerBlocks: _uncontrolledInnerBlocks,
+				controlledInnerBlocks: _controlledInnerBlocks,
 				isInnerBlockSelected: hasSelectedInnerBlock( clientId, true ),
 			};
 		},
 		[ clientId ]
 	);
-	const hasExistingNavItems = !! innerBlocks.length;
 	const {
 		replaceInnerBlocks,
 		selectBlock,
@@ -178,10 +199,10 @@ function Navigation( {
 		setHasSavedUnsavedInnerBlocks,
 	] = useState( false );
 
-	const isWithinUnassignedArea = navigationArea && ! ref;
+	const isWithinUnassignedArea = !! navigationArea && ! ref;
 
 	const [ isPlaceholderShown, setIsPlaceholderShown ] = useState(
-		! hasExistingNavItems || isWithinUnassignedArea
+		! hasUncontrolledInnerBlocks || isWithinUnassignedArea
 	);
 
 	const [ isResponsiveMenuOpen, setResponsiveMenuVisibility ] = useState(
@@ -303,15 +324,17 @@ function Navigation( {
 		setIsPlaceholderShown( ! isEntityAvailable );
 	}, [ isEntityAvailable ] );
 
-	// If the ref no longer exists the reset the inner blocks
-	// to provide a clean slate.
+	// If the ref no longer exists the reset the inner blocks to provide a
+	// clean slate.
 	useEffect( () => {
-		if ( ref === undefined && innerBlocks.length > 0 ) {
+		if (
+			! hasUncontrolledInnerBlocks &&
+			controlledInnerBlocks?.length > 0 &&
+			ref === undefined
+		) {
 			replaceInnerBlocks( clientId, [] );
 		}
-		// innerBlocks are intentionally not listed as deps. This function is only concerned
-		// with the snapshot from the time when ref became undefined.
-	}, [ clientId, ref, innerBlocks ] );
+	}, [ clientId, ref, hasUncontrolledInnerBlocks, controlledInnerBlocks ] );
 
 	useEffect( () => {
 		const setPermissionsNotice = () => {
@@ -377,23 +400,42 @@ function Navigation( {
 	// Either this block was saved in the content or inserted by a pattern.
 	// Consider this 'unsaved'. Offer an uncontrolled version of inner blocks,
 	// that automatically saves the menu.
-	const hasUnsavedBlocks =
-		hasExistingNavItems && ! isEntityAvailable && ! isWithinUnassignedArea;
+	const hasUnsavedBlocks = hasUncontrolledInnerBlocks && ! isEntityAvailable;
 	if ( hasUnsavedBlocks ) {
 		return (
-			<UnsavedInnerBlocks
-				blockProps={ blockProps }
-				blocks={ innerBlocks }
-				clientId={ clientId }
-				navigationMenus={ navigationMenus }
-				hasSelection={ isSelected || isInnerBlockSelected }
-				hasSavedUnsavedInnerBlocks={ hasSavedUnsavedInnerBlocks }
-				onSave={ ( post ) => {
-					setHasSavedUnsavedInnerBlocks( true );
-					// Switch to using the wp_navigation entity.
-					setRef( post.id );
-				} }
-			/>
+			<nav { ...blockProps }>
+				<ResponsiveWrapper
+					id={ clientId }
+					onToggle={ setResponsiveMenuVisibility }
+					isOpen={ isResponsiveMenuOpen }
+					isResponsive={ 'never' !== overlayMenu }
+					isHiddenByDefault={ 'always' === overlayMenu }
+					classNames={ overlayClassnames }
+					styles={ overlayStyles }
+				>
+					<UnsavedInnerBlocks
+						blockProps={ blockProps }
+						blocks={ uncontrolledInnerBlocks }
+						clientId={ clientId }
+						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 );
+							// replaceInnerBlocks is required to ensure the block editor store is sync'd
+							// to be aware that there are now no inner blocks (as blocks moved to entity).
+							// This should probably happen automatically with useBlockSync
+							// but there appears to be a bug.
+							replaceInnerBlocks( clientId, [] );
+							// Switch to using the wp_navigation entity.
+							setRef( post.id );
+						} }
+					/>
+				</ResponsiveWrapper>
+			</nav>
 		);
 	}
 

diff --git a/packages/block-library/src/navigation/edit/unsaved-inner-blocks.js b/packages/block-library/src/navigation/edit/unsaved-inner-blocks.js
@@ -125,22 +125,18 @@ export default function UnsavedInnerBlocks( {
 	] );
 
 	return (
-		<>
-			<nav { ...blockProps }>
-				<div className="wp-block-navigation__unsaved-changes">
-					<Disabled
-						className={ classnames(
-							'wp-block-navigation__unsaved-changes-overlay',
-							{
-								'is-saving': hasSelection,
-							}
-						) }
-					>
-						<div { ...innerBlocksProps } />
-					</Disabled>
-					{ hasSelection && <Spinner /> }
-				</div>
-			</nav>
-		</>
+		<div className="wp-block-navigation__unsaved-changes">
+			<Disabled
+				className={ classnames(
+					'wp-block-navigation__unsaved-changes-overlay',
+					{
+						'is-saving': hasSelection,
+					}
+				) }
+			>
+				<div { ...innerBlocksProps } />
+			</Disabled>
+			{ hasSelection && <Spinner /> }
+		</div>
 	);
 }
diff --git a/packages/e2e-tests/specs/editor/blocks/__snapshots__/navigation.test.js.snap b/packages/e2e-tests/specs/editor/blocks/__snapshots__/navigation.test.js.snap
@@ -43,3 +43,5 @@ exports[`Navigation placeholder allows a navigation block to be created from exi
 exports[`Navigation placeholder allows a navigation block to be created using existing pages 1`] = `"<!-- wp:page-list /-->"`;
 
 exports[`Navigation placeholder creates an empty navigation block when the selected existing menu is also empty 1`] = `""`;
+
+exports[`Navigation supports navigation blocks that have inner blocks within their markup and converts them to wp_navigation posts 1`] = `"<!-- wp:page-list /-->"`;