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

Navigation block: Create classic menus empty #43190

Closed
wants to merge 8 commits into from
Closed

Navigation block: Create classic menus empty #43190

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
ee08868
add page list fallback, fix tests
draganescu Aug 10, 2022
c2b2ce2
update comment
scruffian Aug 10, 2022
95967f6
PHP linting
draganescu Aug 10, 2022
6d36b44
Update packages/block-library/src/navigation/index.php
scruffian Aug 10, 2022
4c76a0d
Update packages/block-library/src/navigation/index.php
scruffian Aug 10, 2022
a30497c
Update packages/block-library/src/navigation/edit/index.js
scruffian Aug 10, 2022
bb6ee93
Update packages/block-library/src/navigation/index.php
scruffian Aug 10, 2022
100566d
Navigation: Don't populate classic menus until they are saved
scruffian Aug 12, 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
137 changes: 105 additions & 32 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,13 +6,7 @@ import classnames from 'classnames';
/**
* WordPress dependencies
*/
import {
useState,
useEffect,
useRef,
useCallback,
Platform,
} from '@wordpress/element';
import { useState, useEffect, useRef, Platform } from '@wordpress/element';
import {
InspectorControls,
BlockControls,
Expand All @@ -29,7 +23,7 @@ import {
} from '@wordpress/block-editor';
import { EntityProvider } from '@wordpress/core-data';

import { useDispatch, useRegistry } from '@wordpress/data';
import { useDispatch } from '@wordpress/data';
import {
PanelBody,
ToggleControl,
Expand All @@ -41,6 +35,7 @@ import {
} from '@wordpress/components';
import { __, sprintf } from '@wordpress/i18n';
import { speak } from '@wordpress/a11y';
import { createBlock } from '@wordpress/blocks';

/**
* Internal dependencies
Expand Down Expand Up @@ -100,7 +95,6 @@ function Navigation( {

const ref = attributes.ref;

const registry = useRegistry();
const setRef = ( postId ) => {
setAttributes( { ref: postId } );
};
Expand Down Expand Up @@ -210,19 +204,37 @@ function Navigation( {
const navMenuResolvedButMissing =
hasResolvedNavigationMenus && isNavigationMenuMissing;

// Attempt to retrieve and prioritize any existing navigation menu unless
// a specific ref is allocated or the user is explicitly creating a new menu. The aim is
// for the block to "just work" from a user perspective using existing data.
// Attempt to retrieve and prioritize any existing navigation menu unless:
// - the are uncontrolled inner blocks already present in the block.
// - the user is creating a new menu.
// - there are no menus to choose from.
// This attempts to pick the first menu if there is a single Navigation Post. If more
// than 1 exists then use the most recent.
// The aim is for the block to "just work" from a user perspective using existing data.
useEffect( () => {
if (
hasUncontrolledInnerBlocks ||
isCreatingNavigationMenu ||
ref ||
! navigationMenus?.length ||
navigationMenus?.length > 1
! navigationMenus?.length
) {
return;
}

navigationMenus.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
* automatically picked) is available.
* The fallback should not request a save (entity dirty state)
* nor to be undoable, hence why it is marked as non persistent
*/
__unstableMarkNextChangeAsNotPersistent();
setRef( navigationMenus[ 0 ].id );
}, [ navigationMenus ] );

Expand All @@ -235,6 +247,7 @@ function Navigation( {
status: classicMenuConversionStatus,
error: classicMenuConversionError,
value: classicMenuConversionResult,
classicInnerBlocks: classicMenuConversionInnerBlocks,
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 used to pass the innerBlocks to the block from the hook.

Copy link
Contributor

Choose a reason for hiding this comment

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

This changed a bit with #43081 and will change more in #42956.

But I think its ok. convert can return the wp_navigation and the innerBlocks:

const { navigationMenu, innerBlocks } = await convertClassicMenu( classicMenu );
setRef( navigationMenu.id );
replaceInnerBlocks( innerBlocks );

} = useConvertClassicToBlockMenu( clientId );

const isConvertingClassicMenu =
Expand All @@ -255,13 +268,24 @@ function Navigation( {
hasResolvedNavigationMenus &&
! hasUncontrolledInnerBlocks;

if ( isPlaceholder && ! ref ) {
/**
* this fallback only displays (both in editor and on front)
* the list of pages block if not menu is available
* we don't want the fallback to request a save
* nor to be undoable, hence we mark it non persistent
*/
__unstableMarkNextChangeAsNotPersistent();
replaceInnerBlocks( clientId, [ createBlock( 'core/page-list' ) ] );
}

const isEntityAvailable =
! isNavigationMenuMissing && isNavigationMenuResolved;

// "loading" state:
// - there is a menu creation process in progress.
// - there is a classic menu conversion process in progress.
// OR
// OR:
// - there is a ref attribute pointing to a Navigation Post
// - the Navigation Post isn't available (hasn't resolved) yet.
const isLoading =
Expand Down Expand Up @@ -336,6 +360,8 @@ function Navigation( {
showClassicMenuConversionNotice(
__( 'Classic menu imported successfully.' )
);

replaceInnerBlocks( clientId, classicMenuConversionInnerBlocks );
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 does work, but other changes get triggered which means they get wiped.

}

if ( classicMenuConversionStatus === CLASSIC_MENU_CONVERSION_ERROR ) {
Expand All @@ -347,6 +373,7 @@ function Navigation( {
classicMenuConversionStatus,
classicMenuConversionResult,
classicMenuConversionError,
classicMenuConversionInnerBlocks,
] );

// Spacer block needs orientation from context. This is a patch until
Expand Down Expand Up @@ -441,17 +468,6 @@ function Navigation( {
shouldFocusNavigationSelector,
] );

const resetToEmptyBlock = useCallback( () => {
registry.batch( () => {
setAttributes( {
ref: undefined,
} );
if ( ! ref ) {
replaceInnerBlocks( clientId, [] );
}
} );
}, [ clientId, ref ] );

const isResponsive = 'never' !== overlayMenu;

const overlayMenuPreviewClasses = classnames(
Expand Down Expand Up @@ -600,6 +616,27 @@ function Navigation( {
if ( hasUnsavedBlocks ) {
return (
<TagName { ...blockProps }>
<BlockControls>
<ToolbarGroup className="wp-block-navigation__toolbar-menu-selector">
<NavigationMenuSelector
ref={ null }
currentMenuId={ null }
clientId={ clientId }
onSelectNavigationMenu={ ( menuId ) => {
handleUpdateMenu( menuId );
setShouldFocusNavigationSelector( true );
} }
onSelectClassicMenu={ ( classicMenu ) => {
convert( classicMenu.id, classicMenu.name );
setShouldFocusNavigationSelector( true );
} }
onCreateNew={ () => createNavigationMenu( '', [] ) }
/* translators: %s: The name of a menu. */
actionLabel={ __( "Switch to '%s'" ) }
showManageActions
/>
</ToolbarGroup>
</BlockControls>
{ stylingInspectorControls }
<ResponsiveWrapper
id={ clientId }
Expand Down Expand Up @@ -639,16 +676,40 @@ function Navigation( {
// TODO - the user should be able to select a new one?
if ( ref && isNavigationMenuMissing ) {
return (
<div { ...blockProps }>
<TagName { ...blockProps }>
<BlockControls>
<ToolbarGroup className="wp-block-navigation__toolbar-menu-selector">
<NavigationMenuSelector
ref={ navigationSelectorRef }
currentMenuId={ ref }
clientId={ clientId }
onSelectNavigationMenu={ ( menuId ) => {
handleUpdateMenu( menuId );
setShouldFocusNavigationSelector( true );
} }
onSelectClassicMenu={ ( classicMenu ) => {
convert( classicMenu.id, classicMenu.name );
setShouldFocusNavigationSelector( true );
} }
onCreateNew={ () => createNavigationMenu( '', [] ) }
/* translators: %s: The name of a menu. */
actionLabel={ __( "Switch to '%s'" ) }
showManageActions
/>
</ToolbarGroup>
</BlockControls>
<Warning>
{ __(
'Navigation menu has been deleted or is unavailable. '
) }
<Button onClick={ resetToEmptyBlock } variant="link">
<Button
onClick={ () => createNavigationMenu( '', [] ) }
variant="link"
>
{ __( 'Create a new menu?' ) }
</Button>
</Warning>
</div>
</TagName>
);
}

Expand All @@ -666,7 +727,17 @@ function Navigation( {
? CustomPlaceholder
: Placeholder;

if ( isPlaceholder ) {
/**
* Historically the navigation block has supported custom placeholders.
* Even though the current UX tries as hard as possible not to
* end up in a placeholder state, the block continues to support
* this extensibility point, via a CustomPlaceholder.
* When CustomPlaceholder is present it becomes the default fallback
* for an empty navigation block, instead of the default fallbacks.
*
*/

if ( isPlaceholder && CustomPlaceholder ) {
return (
<TagName { ...blockProps }>
<PlaceholderComponent
Expand Down Expand Up @@ -709,7 +780,9 @@ function Navigation( {
convert( classicMenu.id, classicMenu.name );
setShouldFocusNavigationSelector( true );
} }
onCreateNew={ resetToEmptyBlock }
onCreateNew={ () =>
createNavigationMenu( '', [] )
}
/* translators: %s: The name of a menu. */
actionLabel={ __( "Switch to '%s'" ) }
showManageActions
Expand All @@ -728,7 +801,7 @@ function Navigation( {
canUserDeleteNavigationMenu && (
<NavigationMenuDeleteControl
onDelete={ ( deletedMenuTitle = '' ) => {
resetToEmptyBlock();
replaceInnerBlocks( clientId, [] );
showNavigationMenuStatusNotice(
sprintf(
// translators: %s: the name of a menu (e.g. Header navigation).
Expand Down
10 changes: 5 additions & 5 deletions packages/block-library/src/navigation/edit/use-convert-classic-menu-to-block-menu.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ function useConvertClassicToBlockMenu( clientId ) {
const [ status, setStatus ] = useState( CLASSIC_MENU_CONVERSION_IDLE );
const [ value, setValue ] = useState( null );
const [ error, setError ] = useState( null );
const [ classicInnerBlocks, setClassicInnerBlocks ] = useState( null );

async function convertClassicMenuToBlockMenu( menuId, menuName ) {
let navigationMenu;
Expand Down Expand Up @@ -65,13 +66,11 @@ function useConvertClassicToBlockMenu( clientId ) {

// 2. Convert the classic items into blocks.
const { innerBlocks } = menuItemsToBlocks( classicMenuItems );
setClassicInnerBlocks( innerBlocks );
Copy link
Contributor Author

Choose a reason for hiding this comment

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

So that they can be passed from the hook back to the component,


// 3. Create the `wp_navigation` Post with the blocks.
try {
navigationMenu = await createNavigationMenu(
menuName,
innerBlocks
);
navigationMenu = await createNavigationMenu( menuName, [] );
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 don't need this change.

} catch ( err ) {
throw new Error(
sprintf(
Expand Down Expand Up @@ -100,7 +99,7 @@ function useConvertClassicToBlockMenu( clientId ) {
setValue( null );
setError( null );

convertClassicMenuToBlockMenu( menuId, menuName )
return convertClassicMenuToBlockMenu( menuId, menuName )
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 don't need this change.

.then( ( navMenu ) => {
setValue( navMenu );
setStatus( CLASSIC_MENU_CONVERSION_SUCCESS );
Expand Down Expand Up @@ -130,6 +129,7 @@ function useConvertClassicToBlockMenu( clientId ) {
status,
value,
error,
classicInnerBlocks,
};
}

Expand Down
1 change: 1 addition & 0 deletions packages/block-library/src/navigation/editor.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -520,6 +520,7 @@ body.editor-styles-wrapper
// so focus is applied naturally on the block container.
// It's important the right container has focus, otherwise you can't press
// "Delete" to remove the block.
.wp-block-navigation__responsive-container,
.wp-block-navigation__responsive-close {
@include break-small() {
pointer-events: none;
Expand Down
25 changes: 10 additions & 15 deletions packages/block-library/src/navigation/index.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -250,29 +250,24 @@ function block_core_navigation_render_submenu_icon() {


/**
* Finds the first non-empty `wp_navigation` Post.
* Finds the most recently published `wp_navigation` Post.
*
* @return WP_Post|null the first non-empty Navigation or null.
*/
function block_core_navigation_get_first_non_empty_navigation() {
// Order and orderby args set to mirror those in `wp_get_nav_menus`
// see:
// - https://github.com/WordPress/wordpress-develop/blob/ba943e113d3b31b121f77a2d30aebe14b047c69d/src/wp-includes/nav-menu.php#L613-L619.
// - https://developer.wordpress.org/reference/classes/wp_query/#order-orderby-parameters.
function block_core_navigation_get_most_recently_published_navigation() {
// We default to the most recently created menu.
$parsed_args = array(
'post_type' => 'wp_navigation',
'no_found_rows' => true,
'order' => 'ASC',
'orderby' => 'name',
'order' => 'DESC',
'orderby' => 'date',
'post_status' => 'publish',
'posts_per_page' => 20, // Try the first 20 posts.
'posts_per_page' => 1, // get only the most recent.
);

$navigation_posts = new WP_Query( $parsed_args );
foreach ( $navigation_posts->posts as $navigation_post ) {
if ( has_blocks( $navigation_post ) ) {
return $navigation_post;
}
$navigation_post = new WP_Query( $parsed_args );
if ( count( $navigation_post->posts ) > 0 ) {
return $navigation_post->posts[0];
}

return null;
Expand Down Expand Up @@ -325,7 +320,7 @@ function block_core_navigation_get_fallback_blocks() {

// Default to a list of Pages.

$navigation_post = block_core_navigation_get_first_non_empty_navigation();
$navigation_post = block_core_navigation_get_most_recently_published_navigation();

// Prefer using the first non-empty Navigation as fallback if available.
if ( $navigation_post ) {
Expand Down
4 changes: 1 addition & 3 deletions packages/e2e-tests/specs/editor/blocks/__snapshots__/navigation.test.js.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@

exports[`Navigation Creating and restarting converts uncontrolled inner blocks to an entity when modifications are made to the blocks 1`] = `"<!-- wp:navigation-link {\\"label\\":\\"A Test Page\\",\\"type\\":\\"page\\",\\"id\\":[number],\\"url\\":\\"http://localhost:8889/?page_id=[number]\\",\\"kind\\":\\"post-type\\"} /-->"`;

exports[`Navigation Placeholder placeholder actions allows a navigation block to be created from existing menus 1`] = `
exports[`Navigation Placeholder menu selector actions allows a navigation block to be created from existing menus 1`] = `
"<!-- wp:navigation-link {\\"label\\":\\"Home\\",\\"type\\":\\"custom\\",\\"url\\":\\"http://localhost:8889/\\",\\"kind\\":\\"custom\\"} /-->

<!-- wp:navigation-submenu {\\"label\\":\\"About\\",\\"type\\":\\"page\\",\\"id\\":[number],\\"url\\":\\"http://localhost:8889/?page_id=[number]\\",\\"kind\\":\\"post-type\\"} -->
Expand Down Expand Up @@ -30,8 +30,6 @@ exports[`Navigation Placeholder placeholder actions allows a navigation block to
<!-- /wp:navigation-submenu -->"
`;

exports[`Navigation Placeholder placeholder actions creates an empty navigation block when the selected existing menu is also empty 1`] = `""`;

exports[`Navigation allows an empty navigation block to be created and manually populated using a mixture of internal and external links 1`] = `
"<!-- wp:navigation-link {\\"label\\":\\"WP\\",\\"url\\":\\"https://wordpress.org\\",\\"kind\\":\\"custom\\",\\"isTopLevelLink\\":true} /-->

Expand Down
Loading