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

Use page list instead of placeholder as fallback #42735

Merged
merged 14 commits into from
Aug 22, 2022
Merged

Use page list instead of placeholder as fallback #42735

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
127e429
add page list fallback, fix tests
draganescu Aug 10, 2022
669e9dc
update comment
scruffian Aug 10, 2022
ae63944
PHP linting
draganescu Aug 10, 2022
80fdee5
Update packages/block-library/src/navigation/index.php
scruffian Aug 10, 2022
38012ef
Update packages/block-library/src/navigation/index.php
scruffian Aug 10, 2022
652499b
Update packages/block-library/src/navigation/edit/index.js
scruffian Aug 10, 2022
da90018
Update packages/block-library/src/navigation/index.php
scruffian Aug 10, 2022
a498287
updated tests to wait for network idle when a menu is created
draganescu Aug 15, 2022
79eddb0
update tests await for clicks and create navigations to test for view JS
draganescu Aug 16, 2022
12bab10
fix auto menu creation for multiple empty blocks inserted
draganescu Aug 16, 2022
ce90988
improve a comment
draganescu Aug 16, 2022
6072b40
fix rebase bad merge of
draganescu Aug 19, 2022
6c67ecd
remove the __unstableIsDisabled prop from the navigation block
scruffian Aug 22, 2022
25eb180
fix error where converting a classic menu did not select the newly cr…
draganescu Aug 22, 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
199 changes: 136 additions & 63 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
draganescu marked this conversation as resolved.
Show resolved Hide resolved
! navigationMenus?.length
) {
return;
}

navigationMenus.sort( ( menuA, menuB ) => {
draganescu marked this conversation as resolved.
Show resolved Hide resolved
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 Down Expand Up @@ -254,13 +266,24 @@ function Navigation( {
hasResolvedNavigationMenus &&
! hasUncontrolledInnerBlocks;

if ( isPlaceholder && ! ref ) {
draganescu marked this conversation as resolved.
Show resolved Hide resolved
/**
* this fallback only displays (both in editor and on front)
* the list of pages block if no menu is available as a fallback.
* 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' ) ] );
Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like this action is dispatched on render, so have to be a little bit careful about accidentally dispatching it multiple times.

Using the inner blocks template property may also be a viable way to define some default blocks. Though I might be missing some aspects of the requirements here.

Copy link
Contributor Author

@draganescu draganescu Aug 15, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

@talldan is this a similar idea?

}

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 All @@ -272,38 +295,30 @@ function Navigation( {
const textDecoration = attributes.style?.typography?.textDecoration;

const hasBlockOverlay = useBlockOverlayActive( clientId );
const blockProps = useBlockProps(
{
ref: navRef,
className: classnames( className, {
'items-justified-right': justifyContent === 'right',
'items-justified-space-between':
justifyContent === 'space-between',
'items-justified-left': justifyContent === 'left',
'items-justified-center': justifyContent === 'center',
'is-vertical': orientation === 'vertical',
'no-wrap': flexWrap === 'nowrap',
'is-responsive': 'never' !== overlayMenu,
'has-text-color': !! textColor.color || !! textColor?.class,
[ getColorClassName( 'color', textColor?.slug ) ]:
!! textColor?.slug,
'has-background':
!! backgroundColor.color || backgroundColor.class,
[ getColorClassName(
'background-color',
backgroundColor?.slug
) ]: !! backgroundColor?.slug,
[ `has-text-decoration-${ textDecoration }` ]: textDecoration,
'block-editor-block-content-overlay': hasBlockOverlay,
} ),
style: {
color: ! textColor?.slug && textColor?.color,
backgroundColor:
! backgroundColor?.slug && backgroundColor?.color,
},
const blockProps = useBlockProps( {
ref: navRef,
className: classnames( className, {
'items-justified-right': justifyContent === 'right',
'items-justified-space-between': justifyContent === 'space-between',
'items-justified-left': justifyContent === 'left',
'items-justified-center': justifyContent === 'center',
'is-vertical': orientation === 'vertical',
'no-wrap': flexWrap === 'nowrap',
'is-responsive': 'never' !== overlayMenu,
'has-text-color': !! textColor.color || !! textColor?.class,
[ getColorClassName( 'color', textColor?.slug ) ]:
!! textColor?.slug,
'has-background': !! backgroundColor.color || backgroundColor.class,
[ getColorClassName( 'background-color', backgroundColor?.slug ) ]:
!! backgroundColor?.slug,
[ `has-text-decoration-${ textDecoration }` ]: textDecoration,
'block-editor-block-content-overlay': hasBlockOverlay,
} ),
style: {
color: ! textColor?.slug && textColor?.color,
backgroundColor: ! backgroundColor?.slug && backgroundColor?.color,
},
{ __unstableIsDisabled: hasBlockOverlay }
);
} );

// Turn on contrast checker for web only since it's not supported on mobile yet.
const enableContrastChecking = Platform.OS === 'web';
Expand Down Expand Up @@ -432,17 +447,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 @@ -591,6 +595,33 @@ 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={ async ( classicMenu ) => {
const navMenu = await convertClassicMenu(
classicMenu.id,
classicMenu.name
);
if ( navMenu ) {
handleUpdateMenu( navMenu.id );
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 @@ -630,16 +661,46 @@ 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={ async ( classicMenu ) => {
const navMenu = await convertClassicMenu(
classicMenu.id,
classicMenu.name
);
if ( navMenu ) {
handleUpdateMenu( navMenu.id );
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 @@ -657,7 +718,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 @@ -714,7 +785,9 @@ function Navigation( {
);
}
} }
onCreateNew={ resetToEmptyBlock }
onCreateNew={ () =>
createNavigationMenu( '', [] )
draganescu marked this conversation as resolved.
Show resolved Hide resolved
}
/* translators: %s: The name of a menu. */
actionLabel={ __( "Switch to '%s'" ) }
showManageActions
Expand All @@ -733,7 +806,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
4 changes: 3 additions & 1 deletion 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 @@ -63,7 +63,9 @@ export default function UnsavedInnerBlocks( {
// from the original inner blocks from the post content then the
// user has made changes to the inner blocks. At this point the inner
// blocks can be considered "dirty".
const innerBlocksAreDirty = blocks !== originalBlocks.current;
// We also make sure the current innerBlocks had a chance to be set.
const innerBlocksAreDirty =
!! originalBlocks.current && blocks !== originalBlocks.current;

const shouldDirectInsert = useMemo(
() =>
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;
}
draganescu marked this conversation as resolved.
Show resolved Hide resolved
$navigation_post = new WP_Query( $parsed_args );
if ( count( $navigation_post->posts ) > 0 ) {
return $navigation_post->posts[0];
draganescu marked this conversation as resolved.
Show resolved Hide resolved
}

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: 2 additions & 2 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,7 +30,7 @@ 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 Placeholder menu selector 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