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

Improve Nav block loading and placeholder states #38907

Merged
merged 23 commits into from
Mar 2, 2022
Merged

Improve Nav block loading and placeholder states #38907

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
0343640
Improve all loading statess
getdave Feb 17, 2022
9d35153
Squash and rebase
getdave Mar 2, 2022
1d75aa8
Translate spoken strings
getdave Feb 23, 2022
afde8ea
Coerce to Boolean at source
getdave Feb 28, 2022
653175e
Correct typo
getdave Feb 28, 2022
a66247b
Refactor to use radio control component for block based menu selector
getdave Feb 28, 2022
1e2dbf5
Group placeholder tests
getdave Feb 28, 2022
d128e60
Remove unnecessary usage of classnames util
getdave Feb 28, 2022
e31abdf
Fix inner blocks placeholder to be visible by ARIA
getdave Feb 28, 2022
b613843
Only set aria-hidden where necessary
getdave Feb 28, 2022
4fa7c43
Remove developer centric term from placeholder announcements
getdave Feb 28, 2022
a155cfb
Remove default polite arg to speak function
getdave Feb 28, 2022
df1467a
Reverse conditional to aid in clarity
getdave Feb 28, 2022
062068f
Add clarity to test purpose and method via comments
getdave Feb 28, 2022
0f9cbfe
Improve test comprehension via comments
getdave Feb 28, 2022
eab4ad4
Qualify test comment to explain purpose
getdave Mar 1, 2022
322ba64
Update snapshots
getdave Mar 1, 2022
8665ec6
Remove early return for loading state
getdave Mar 1, 2022
96d69d4
Reduce verbosity of vars and colocate
getdave Mar 1, 2022
78997b7
Add spacing around spinner when block is selected
getdave Mar 1, 2022
a361049
Remove isPlaceholder checks when already confirmed
getdave Mar 1, 2022
7ccc01f
Optimise props
getdave Mar 1, 2022
2886dac
Check for possible empty menus values
getdave Mar 2, 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
99 changes: 53 additions & 46 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 @@ -36,6 +36,7 @@ import {
__experimentalToggleGroupControlOption as ToggleGroupControlOption,
ToolbarGroup,
Button,
Spinner,
} from '@wordpress/components';
import { __ } from '@wordpress/i18n';

Expand All @@ -46,7 +47,6 @@ import useListViewModal from './use-list-view-modal';
import useNavigationMenu from '../use-navigation-menu';
import useNavigationEntities from '../use-navigation-entities';
import Placeholder from './placeholder';
import PlaceholderPreview from './placeholder/placeholder-preview';
import ResponsiveWrapper from './responsive-wrapper';
import NavigationInnerBlocks from './inner-blocks';
import NavigationMenuSelector from './navigation-menu-selector';
Expand Down Expand Up @@ -172,8 +172,7 @@ function Navigation( {
// introduce a selector like `getUncontrolledInnerBlocks`, just in
// case `getBlock` is fixed.
const _uncontrolledInnerBlocks = getBlock( clientId ).innerBlocks;
const _hasUncontrolledInnerBlocks =
_uncontrolledInnerBlocks?.length;
const _hasUncontrolledInnerBlocks = !! _uncontrolledInnerBlocks?.length;
const _controlledInnerBlocks = _hasUncontrolledInnerBlocks
? EMPTY_ARRAY
: getBlocks( clientId );
Expand Down Expand Up @@ -205,10 +204,6 @@ function Navigation( {

const isWithinUnassignedArea = !! navigationArea && ! ref;

const [ isPlaceholderShown, setIsPlaceholderShown ] = useState(
! hasUncontrolledInnerBlocks || isWithinUnassignedArea
);

const [ isResponsiveMenuOpen, setResponsiveMenuVisibility ] = useState(
false
);
Expand All @@ -218,15 +213,14 @@ function Navigation( {
const {
isNavigationMenuResolved,
isNavigationMenuMissing,
canSwitchNavigationMenu,
hasResolvedNavigationMenus,
navigationMenus,
navigationMenu,
canUserUpdateNavigationMenu,
hasResolvedCanUserUpdateNavigationMenu,
canUserDeleteNavigationMenu,
hasResolvedCanUserDeleteNavigationMenu,
canUserCreateNavigationMenu,
isResolvingCanUserCreateNavigationMenu,
hasResolvedCanUserCreateNavigationMenu,
} = useNavigationMenu( ref );

Expand All @@ -237,9 +231,24 @@ function Navigation( {
clientId
);

// The standard HTML5 tag for the block wrapper.
const TagName = 'nav';

// "placeholder" shown if:
// - we don't have a ref attribute pointing to a Navigation Post.
// - we don't have uncontrolled blocks.
// - (legacy) we have a Navigation Area without a ref attribute pointing to a Navigation Post.
const isPlaceholder =
! ref && ( ! hasUncontrolledInnerBlocks || isWithinUnassignedArea );

const isEntityAvailable =
! isNavigationMenuMissing && isNavigationMenuResolved;

// "loading" state:
// - there is a ref attribute pointing to a Navigation Post
// - the Navigation Post isn't available (hasn't resolved) yet.
const isLoading = !! ( ref && ! isEntityAvailable );

const blockProps = useBlockProps( {
ref: navRef,
className: classnames( className, {
Expand Down Expand Up @@ -331,11 +340,6 @@ function Navigation( {
}
} );

// Hide the placeholder if an navigation menu entity has loaded.
useEffect( () => {
setIsPlaceholderShown( ! isEntityAvailable );
}, [ isEntityAvailable ] );

const [ showCantEditNotice, hideCantEditNotice ] = useNavigationNotice( {
name: 'block-library/core/navigation/permissions/update',
message: __(
Expand Down Expand Up @@ -395,7 +399,6 @@ function Navigation( {
if ( ! ref ) {
replaceInnerBlocks( clientId, [] );
}
setIsPlaceholderShown( true );
} );
}, [ clientId, ref ] );

Expand All @@ -407,7 +410,7 @@ function Navigation( {
const hasUnsavedBlocks = hasUncontrolledInnerBlocks && ! isEntityAvailable;
if ( hasUnsavedBlocks ) {
return (
<nav { ...blockProps }>
<TagName { ...blockProps }>
<ResponsiveWrapper
id={ clientId }
onToggle={ setResponsiveMenuVisibility }
Expand All @@ -434,7 +437,7 @@ function Navigation( {
} }
/>
</ResponsiveWrapper>
</nav>
</TagName>
);
}

Expand Down Expand Up @@ -476,6 +479,28 @@ function Navigation( {
{ open: overlayMenuPreview }
);

if ( isPlaceholder ) {
return (
<TagName { ...blockProps }>
<PlaceholderComponent
isSelected={ isSelected }
currentMenuId={ ref }
clientId={ clientId }
canUserCreateNavigationMenu={ canUserCreateNavigationMenu }
isResolvingCanUserCreateNavigationMenu={
isResolvingCanUserCreateNavigationMenu
}
onFinish={ ( post ) => {
if ( post ) {
setRef( post.id );
}
selectBlock( clientId );
} }
/>
</TagName>
);
}

return (
<EntityProvider kind="postType" type="wp_navigation" id={ ref }>
<RecursionProvider>
Expand Down Expand Up @@ -648,32 +673,15 @@ function Navigation( {
) }
</InspectorControls>
) }
<nav { ...blockProps }>
{ isPlaceholderShown && (
Copy link
Contributor Author

Choose a reason for hiding this comment

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

These complex loading states have been extracted to separate conditionals each of which returns early with a dedicated render.

<PlaceholderComponent
currentMenuId={ ref }
onFinish={ ( post ) => {
setIsPlaceholderShown( false );
if ( post ) {
setRef( post.id );
}
selectBlock( clientId );
} }
canSwitchNavigationMenu={ canSwitchNavigationMenu }
hasResolvedNavigationMenus={
hasResolvedNavigationMenus
}
clientId={ clientId }
canUserCreateNavigationMenu={
canUserCreateNavigationMenu
}
/>
) }
{ ! hasResolvedCanUserCreateNavigationMenu ||
( ! isEntityAvailable && ! isPlaceholderShown && (
<PlaceholderPreview isLoading />
) ) }
{ ! isPlaceholderShown && (

{ isLoading && (
<TagName { ...blockProps }>
<Spinner className="wp-block-navigation__loading-indicator" />
</TagName>
) }

{ ! isLoading && (
<TagName { ...blockProps }>
<ResponsiveWrapper
id={ clientId }
onToggle={ setResponsiveMenuVisibility }
Expand All @@ -687,7 +695,6 @@ function Navigation( {
>
{ isEntityAvailable && (
<NavigationInnerBlocks
isVisible={ ! isPlaceholderShown }
clientId={ clientId }
hasCustomPlaceholder={
!! CustomPlaceholder
Expand All @@ -696,8 +703,8 @@ function Navigation( {
/>
) }
</ResponsiveWrapper>
) }
</nav>
</TagName>
) }
</RecursionProvider>
</EntityProvider>
);
Expand Down
13 changes: 10 additions & 3 deletions packages/block-library/src/navigation/edit/inner-blocks.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,6 @@ const LAYOUT = {
};

export default function NavigationInnerBlocks( {
isVisible,
clientId,
hasCustomPlaceholder,
orientation,
Expand Down Expand Up @@ -98,6 +97,15 @@ export default function NavigationInnerBlocks( {

const placeholder = useMemo( () => <PlaceholderPreview />, [] );

const hasMenuItems = !! blocks?.length;

// If there is a `ref` attribute pointing to a `wp_navigation` but
// that menu has no **items** (i.e. empty) then show a placeholder.
// The block must also be selected else the placeholder will display
// alongside the appender.
const showPlaceholder =
! hasCustomPlaceholder && ! hasMenuItems && ! isSelected;

const innerBlocksProps = useInnerBlocksProps(
{
className: 'wp-block-navigation__container',
Expand Down Expand Up @@ -130,8 +138,7 @@ export default function NavigationInnerBlocks( {
// inherit templateLock={ 'all' }.
templateLock: false,
__experimentalLayout: LAYOUT,
placeholder:
! isVisible || hasCustomPlaceholder ? undefined : placeholder,
placeholder: showPlaceholder ? placeholder : undefined,
}
);

Expand Down
60 changes: 32 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 @@ -4,11 +4,13 @@
import {
MenuGroup,
MenuItem,
MenuItemsChoice,
ToolbarDropdownMenu,
} from '@wordpress/components';
import { __, sprintf } from '@wordpress/i18n';
import { decodeEntities } from '@wordpress/html-entities';
import { addQueryArgs } from '@wordpress/url';
import { useCallback, useMemo } from '@wordpress/element';

/**
* Internal dependencies
Expand Down Expand Up @@ -41,12 +43,6 @@ export default function NavigationMenuSelector( {
canSwitchNavigationMenu,
} = useNavigationMenu();

// Avoid showing any currently active menu in the list of
// menus that can be selected.
const navigationMenusOmitCurrent = navigationMenus?.filter(
( menu ) => ! currentMenuId || menu.id !== currentMenuId
);

const createNavigationMenu = useCreateNavigationMenu( clientId );

const onFinishMenuCreation = async (
Expand All @@ -68,7 +64,30 @@ export default function NavigationMenuSelector( {
onFinishMenuCreation
);

const hasNavigationMenus = !! navigationMenusOmitCurrent?.length;
const handleSelect = useCallback(
( _onClose ) => ( selectedId ) => {
_onClose();
onSelect(
navigationMenus?.find( ( post ) => post.id === selectedId )
);
},
[ navigationMenus ]
);

const menuChoices = useMemo( () => {
return (
navigationMenus?.map( ( { id, title } ) => {
const label = decodeEntities( title.rendered );
return {
value: id,
label,
ariaLabel: sprintf( actionLabel, label ),
};
} ) || []
);
}, [ navigationMenus ] );

const hasNavigationMenus = !! navigationMenus?.length;
const hasClassicMenus = !! classicMenus?.length;
const showNavigationMenus = !! canSwitchNavigationMenu;
const showClassicMenus = !! canUserCreateNavigationMenu;
Expand Down Expand Up @@ -98,31 +117,16 @@ export default function NavigationMenuSelector( {
<>
{ showNavigationMenus && hasNavigationMenus && (
<MenuGroup label={ __( 'Menus' ) }>
{ navigationMenusOmitCurrent.map( ( menu ) => {
const label = decodeEntities(
menu.title.rendered
);
return (
<MenuItem
onClick={ () => {
onClose();
onSelect( menu );
} }
key={ menu.id }
aria-label={ sprintf(
actionLabel,
label
) }
>
{ label }
</MenuItem>
);
} ) }
<MenuItemsChoice
getdave marked this conversation as resolved.
Show resolved Hide resolved
value={ currentMenuId }
onSelect={ handleSelect( onClose ) }
choices={ menuChoices }
/>
</MenuGroup>
) }
{ showClassicMenus && hasClassicMenus && (
<MenuGroup label={ __( 'Classic Menus' ) }>
{ classicMenus.map( ( menu ) => {
{ classicMenus?.map( ( menu ) => {
const label = decodeEntities( menu.name );
return (
<MenuItem
Expand Down
Loading