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.

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: Use the ListView in the Navigation block inspector controls #49417

Merged
merged 9 commits into from
May 17, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
9 changes: 7 additions & 2 deletions packages/block-editor/src/components/list-view/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -264,18 +264,23 @@ function ListViewComponent(
</AsyncModeProvider>
);
}

// This is the private API for the ListView component.
// It allows access to all props, not just the public ones.
export const PrivateListView = forwardRef( ListViewComponent );

// This is the public API for the ListView component.
// We wrap the PrivateListView component to hide some props from the public API.
export default forwardRef( ( props, ref ) => {
return (
<PrivateListView
ref={ ref }
{ ...props }
showAppender={ false }
blockSettingsMenu={ BlockSettingsDropdown }
Copy link
Contributor

@ajlende ajlende May 11, 2023

Choose a reason for hiding this comment

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

I removed this in my commit because BlockSettingsDropdown is the default value, and it meant that there were fewer things that needed to be imported here.

This change does mean that it can be overridden by props passed to ListView now, so if that shouldn't be possible, then it needs to be added back as undefined or something.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah we can't do that because the point it to restrict this from being prop being used by third parties. undefined should work though, I'll push that now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done in 74a8ac2

rootClientId={ null }
onSelect={ null }
renderAdditionalBlockUICallback={ null }
renderAdditionalBlockUI={ null }
blockSettingsMenu={ undefined }
/>
);
} );
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
/**
* WordPress dependencies
*/
import { safeDecodeURI } from '@wordpress/url';
import { escapeHTML } from '@wordpress/escape-html';
import { safeDecodeURI } from '@wordpress/url';

/**
* @typedef {'post-type'|'custom'|'taxonomy'|'post-type-archive'} WPNavigationLinkKind
Expand Down Expand Up @@ -89,7 +89,7 @@ export const updateAttributes = (

setAttributes( {
// Passed `url` may already be encoded. To prevent double encoding, decodeURI is executed to revert to the original string.
...{ url: newUrl ? encodeURI( safeDecodeURI( newUrl ) ) : newUrl },
...( newUrl && { url: encodeURI( safeDecodeURI( newUrl ) ) } ),
...( label && { label } ),
...( undefined !== opensInNewTab && { opensInNewTab } ),
...( id && Number.isInteger( id ) && { id } ),
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,43 @@
/**
* WordPress dependencies
*/
import { useSelect, useDispatch } from '@wordpress/data';
import { store as blockEditorStore } from '@wordpress/block-editor';

export const useInsertedBlock = ( insertedBlockClientId ) => {
const { insertedBlockAttributes, insertedBlockName } = useSelect(
( select ) => {
const { getBlockName, getBlockAttributes } =
select( blockEditorStore );

return {
insertedBlockAttributes: getBlockAttributes(
insertedBlockClientId
),
insertedBlockName: getBlockName( insertedBlockClientId ),
};
},
[ insertedBlockClientId ]
);

const { updateBlockAttributes } = useDispatch( blockEditorStore );

const setInsertedBlockAttributes = ( _updatedAttributes ) => {
if ( ! insertedBlockClientId ) return;
updateBlockAttributes( insertedBlockClientId, _updatedAttributes );
};

if ( ! insertedBlockClientId ) {
return {
insertedBlockAttributes: undefined,
insertedBlockName: undefined,
setInsertedBlockAttributes,
};
}

return {
insertedBlockAttributes,
insertedBlockName,
setInsertedBlockAttributes,
};
};
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import {
Spinner,
} from '@wordpress/components';
import { useSelect } from '@wordpress/data';
import { useState, useEffect } from '@wordpress/element';
import { __, sprintf } from '@wordpress/i18n';

/**
Expand All @@ -23,9 +24,16 @@ import { unlock } from '../../private-apis';
import DeletedNavigationWarning from './deleted-navigation-warning';
import useNavigationMenu from '../use-navigation-menu';
import LeafMoreMenu from './leaf-more-menu';
import { updateAttributes } from '../../navigation-link/update-attributes';
import { LinkUI } from '../../navigation-link/link-ui';
import { useInsertedBlock } from '../../navigation-link/use-inserted-block';

/* translators: %s: The name of a menu. */
const actionLabel = __( "Switch to '%s'" );
const BLOCKS_WITH_LINK_UI_SUPPORT = [
'core/navigation-link',
'core/navigation-submenu',
];

const MainContent = ( {
clientId,
Expand All @@ -34,7 +42,7 @@ const MainContent = ( {
isNavigationMenuMissing,
onCreateNew,
} ) => {
const { OffCanvasEditor } = unlock( blockEditorPrivateApis );
const { PrivateListView } = unlock( blockEditorPrivateApis );

// Provide a hierarchy of clientIds for the given Navigation block (clientId).
// This is required else the list view will display the entire block tree.
Expand All @@ -45,6 +53,42 @@ const MainContent = ( {
},
[ clientId ]
);

const [ clientIdWithOpenLinkUI, setClientIdWithOpenLinkUI ] = useState();
const { lastInsertedBlockClientId } = useSelect( ( select ) => {
const { getLastInsertedBlocksClientIds } = unlock(
select( blockEditorStore )
);
const lastInsertedBlocksClientIds = getLastInsertedBlocksClientIds();
return {
lastInsertedBlockClientId:
lastInsertedBlocksClientIds && lastInsertedBlocksClientIds[ 0 ],
};
}, [] );

const {
insertedBlockAttributes,
insertedBlockName,
setInsertedBlockAttributes,
} = useInsertedBlock( lastInsertedBlockClientId );

const hasExistingLinkValue = insertedBlockAttributes?.url;

useEffect( () => {
if (
lastInsertedBlockClientId &&
BLOCKS_WITH_LINK_UI_SUPPORT?.includes( insertedBlockName ) &&
! hasExistingLinkValue // don't re-show the Link UI if the block already has a link value.
) {
setClientIdWithOpenLinkUI( lastInsertedBlockClientId );
}
}, [
lastInsertedBlockClientId,
clientId,
insertedBlockName,
hasExistingLinkValue,
] );

const { navigationMenu } = useNavigationMenu( currentMenuId );

if ( currentMenuId && isNavigationMenuMissing ) {
Expand All @@ -59,19 +103,46 @@ const MainContent = ( {
? sprintf(
/* translators: %s: The name of a menu. */
__( 'Structure for navigation menu: %s' ),
navigationMenu?.title || __( 'Untitled menu' )
navigationMenu?.title?.rendered || __( 'Untitled menu' )
)
: __(
'You have not yet created any menus. Displaying a list of your Pages'
);

const renderLinkUI = ( block ) => {
return (
clientIdWithOpenLinkUI === block.clientId && (
<LinkUI
clientId={ lastInsertedBlockClientId }
link={ insertedBlockAttributes }
onClose={ () => setClientIdWithOpenLinkUI( null ) }
hasCreateSuggestion={ false }
onChange={ ( updatedValue ) => {
updateAttributes(
updatedValue,
setInsertedBlockAttributes,
insertedBlockAttributes
);
setClientIdWithOpenLinkUI( null );
} }
onCancel={ () => setClientIdWithOpenLinkUI( null ) }
/>
)
);
};

return (
<OffCanvasEditor
blocks={ clientIdsTree }
parentClientId={ clientId }
isExpanded={ true }
LeafMoreMenu={ LeafMoreMenu }
description={ description }
/>
<div className="wp-block-navigation__menu-inspector-controls">
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can we avoid this extra div?

Copy link
Contributor

@MaggieCabrera MaggieCabrera May 11, 2023

Choose a reason for hiding this comment

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

no, we need the classname on a div, not the table element for the horizontal scroll to work

<PrivateListView
blocks={ clientIdsTree }
rootClientId={ clientId }
isExpanded
description={ description }
showAppender
blockSettingsMenu={ LeafMoreMenu }
renderAdditionalBlockUI={ renderLinkUI }
/>
</div>
);
};

Expand Down
6 changes: 6 additions & 0 deletions packages/block-library/src/navigation/editor.scss
Original file line number Diff line number Diff line change
Expand Up @@ -657,3 +657,9 @@ body.editor-styles-wrapper .wp-block-navigation__responsive-container.is-menu-op
.wp-block-navigation__responsive-container-open.components-button {
opacity: 1;
}

.wp-block-navigation__menu-inspector-controls {
overflow-x: auto;

@include custom-scrollbars-on-hover(transparent, $gray-600);
MaggieCabrera marked this conversation as resolved.
Show resolved Hide resolved
}
39 changes: 21 additions & 18 deletions test/e2e/specs/editor/blocks/navigation.spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -483,7 +483,7 @@ test.describe( 'Navigation block', () => {
await expect(
listView
.getByRole( 'gridcell', {
name: 'Page Link link',
name: 'Page Link',
} )
.filter( {
hasText: 'Block 1 of 2, Level 1', // proxy for filtering by description.
Expand All @@ -494,7 +494,7 @@ test.describe( 'Navigation block', () => {
await expect(
listView
.getByRole( 'gridcell', {
name: 'Submenu link',
name: 'Submenu',
} )
.filter( {
hasText: 'Block 2 of 2, Level 1', // proxy for filtering by description.
Expand All @@ -505,7 +505,7 @@ test.describe( 'Navigation block', () => {
await expect(
listView
.getByRole( 'gridcell', {
name: 'Page Link link',
name: 'Page Link',
} )
.filter( {
hasText: 'Block 1 of 1, Level 2', // proxy for filtering by description.
Expand Down Expand Up @@ -588,7 +588,7 @@ test.describe( 'Navigation block', () => {
await expect(
listView
.getByRole( 'gridcell', {
name: 'Page Link link',
name: 'Page Link',
} )
.filter( {
hasText: 'Block 3 of 3, Level 1', // proxy for filtering by description.
Expand Down Expand Up @@ -616,7 +616,7 @@ test.describe( 'Navigation block', () => {
} );

const submenuOptions = listView.getByRole( 'button', {
name: 'Options for Submenu block',
name: 'Options for Submenu',
} );

// Open the options menu.
Expand All @@ -626,7 +626,7 @@ test.describe( 'Navigation block', () => {
// outside of the treegrid.
const removeBlockOption = page
.getByRole( 'menu', {
name: 'Options for Submenu block',
name: 'Options for Submenu',
} )
.getByRole( 'menuitem', {
name: 'Remove Top Level Item 2',
Expand All @@ -638,7 +638,7 @@ test.describe( 'Navigation block', () => {
await expect(
listView
.getByRole( 'gridcell', {
name: 'Submenu link',
name: 'Submenu',
} )
.filter( {
hasText: 'Block 2 of 2, Level 1', // proxy for filtering by description.
Expand Down Expand Up @@ -666,10 +666,12 @@ test.describe( 'Navigation block', () => {
} );

// Click on the first menu item to open its settings.
const firstMenuItemAnchor = listView.getByRole( 'link', {
name: 'Top Level Item 1',
includeHidden: true,
} );
const firstMenuItemAnchor = listView
.getByRole( 'link', {
name: 'Page',
includeHidden: true,
} )
.getByText( 'Top Level Item 1' );
await firstMenuItemAnchor.click();

// Get the settings panel.
Expand Down Expand Up @@ -730,7 +732,7 @@ test.describe( 'Navigation block', () => {
await expect(
listViewPanel
.getByRole( 'gridcell', {
name: 'Page Link link',
name: 'Page Link',
} )
.filter( {
hasText: 'Block 1 of 2, Level 1', // proxy for filtering by description.
Expand Down Expand Up @@ -761,7 +763,7 @@ test.describe( 'Navigation block', () => {
// click on options menu for the first menu item and select remove.
const firstMenuItem = listView
.getByRole( 'gridcell', {
name: 'Page Link link',
name: 'Page Link',
} )
.filter( {
hasText: 'Block 1 of 2, Level 1', // proxy for filtering by description.
Expand All @@ -771,7 +773,7 @@ test.describe( 'Navigation block', () => {
const firstItemOptions = firstMenuItem
.locator( '..' ) // parent selector.
.getByRole( 'button', {
name: 'Options for Page Link block',
name: 'Options for Page Link',
} );

// Open the options menu.
Expand All @@ -782,10 +784,10 @@ test.describe( 'Navigation block', () => {
// outside of the treegrid.
const addSubmenuOption = page
.getByRole( 'menu', {
name: 'Options for Page Link block',
name: 'Options for Page Link',
} )
.getByRole( 'menuitem', {
name: 'Add submenu link',
name: 'Add submenu',
} );

await addSubmenuOption.click();
Expand All @@ -798,7 +800,7 @@ test.describe( 'Navigation block', () => {
await expect(
listView
.getByRole( 'gridcell', {
name: 'Custom Link link',
name: 'Custom Link',
} )
.filter( {
hasText: 'Block 1 of 1, Level 2', // proxy for filtering by description.
Expand All @@ -811,7 +813,7 @@ test.describe( 'Navigation block', () => {
await expect(
listView
.getByRole( 'gridcell', {
name: 'Submenu link',
name: 'Submenu',
} )
.filter( {
hasText: 'Block 1 of 2, Level 1', // proxy for filtering by description.
Expand Down Expand Up @@ -1030,6 +1032,7 @@ test.describe( 'Navigation block - Frontend interactivity', () => {
} );
const innerElement = page.getByRole( 'link', {
name: 'Simple Submenu Link 1',
includeHidden: true,
} );
await expect( innerElement ).toBeHidden();
await simpleSubmenuButton.click();
Expand Down