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

Duplicate LeafMoreMenu into the navigation block and the global sidebar navigation #50489

Merged
merged 3 commits into from
May 11, 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
3 changes: 3 additions & 0 deletions packages/block-editor/src/components/list-view/block.js
Original file line number Diff line number Diff line change
Expand Up @@ -134,6 +134,7 @@ function ListViewBlock( {
collapse,
BlockSettingsMenu,
listViewInstanceId,
expandedState,
} = useListViewContext();

const hasSiblings = siblingBlockCount > 0;
Expand Down Expand Up @@ -336,6 +337,8 @@ function ListViewBlock( {
} }
disableOpenOnArrowDown
__experimentalSelectBlock={ updateSelection }
expand={ expand }
expandedState={ expandedState }
/>
) }
</TreeGridCell>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -124,7 +124,7 @@ function ListViewBlock( {
[ selectBlock ]
);

const { isTreeGridMounted, expand, collapse, LeafMoreMenu } =
const { isTreeGridMounted, expand, expandedState, collapse, LeafMoreMenu } =
useListViewContext();

const toggleExpanded = useCallback(
Expand Down Expand Up @@ -334,6 +334,8 @@ function ListViewBlock( {
__experimentalSelectBlock={
updateSelection
}
expandedState={ expandedState }
expand={ expand }
/>
</>
) }
Expand Down
2 changes: 0 additions & 2 deletions packages/block-editor/src/private-apis.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@ import * as globalStyles from './components/global-styles';
import { ExperimentalBlockEditorProvider } from './components/provider';
import { lock } from './lock-unlock';
import OffCanvasEditor from './components/off-canvas-editor';
import LeafMoreMenu from './components/off-canvas-editor/leaf-more-menu';
import ResizableBoxPopover from './components/resizable-box-popover';
import { ComposedPrivateInserter as PrivateInserter } from './components/inserter';
import { PrivateListView } from './components/list-view';
Expand All @@ -20,7 +19,6 @@ export const privateApis = {};
lock( privateApis, {
...globalStyles,
ExperimentalBlockEditorProvider,
LeafMoreMenu,
OffCanvasEditor,
PrivateInserter,
PrivateListView,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,13 +11,7 @@ import {
import { DropdownMenu, MenuItem, MenuGroup } from '@wordpress/components';
import { useDispatch, useSelect } from '@wordpress/data';
import { __, sprintf } from '@wordpress/i18n';

/**
* Internal dependencies
*/
import { store as blockEditorStore } from '../../store';
import BlockTitle from '../block-title';
import { useListViewContext } from './context';
import { BlockTitle, store as blockEditorStore } from '@wordpress/block-editor';

const POPOVER_PROPS = {
className: 'block-editor-block-settings-menu__popover',
Expand All @@ -30,8 +24,7 @@ const BLOCKS_THAT_CAN_BE_CONVERTED_TO_SUBMENU = [
'core/navigation-submenu',
];

function AddSubmenuItem( { block, onClose } ) {
const { expandedState, expand } = useListViewContext();
function AddSubmenuItem( { block, onClose, expandedState, expand } ) {
const { insertBlock, replaceBlock, replaceInnerBlocks } =
useDispatch( blockEditorStore );

Expand Down Expand Up @@ -139,7 +132,13 @@ export default function LeafMoreMenu( props ) {
>
{ __( 'Move down' ) }
</MenuItem>
<AddSubmenuItem block={ block } onClose={ onClose } />
<AddSubmenuItem
block={ block }
onClose={ onClose }
expanded
expandedState={ props.expandedState }
expand={ props.expand }
/>
</MenuGroup>
<MenuGroup>
<MenuItem
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,10 +22,10 @@ import NavigationMenuSelector from './navigation-menu-selector';
import { unlock } from '../../private-apis';
import DeletedNavigationWarning from './deleted-navigation-warning';
import useNavigationMenu from '../use-navigation-menu';
import LeafMoreMenu from './leaf-more-menu';

/* translators: %s: The name of a menu. */
const actionLabel = __( "Switch to '%s'" );
const { OffCanvasEditor, LeafMoreMenu } = unlock( blockEditorPrivateApis );

const MainContent = ( {
clientId,
Expand All @@ -34,6 +34,8 @@ const MainContent = ( {
isNavigationMenuMissing,
onCreateNew,
} ) => {
const { OffCanvasEditor } = unlock( blockEditorPrivateApis );
Copy link
Contributor

Choose a reason for hiding this comment

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

why did we move this?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure :)


// Provide a hierarchy of clientIds for the given Navigation block (clientId).
// This is required else the list view will display the entire block tree.
const clientIdsTree = useSelect(
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,156 @@
/**
* WordPress dependencies
*/
import { createBlock } from '@wordpress/blocks';
import {
addSubmenu,
chevronUp,
chevronDown,
moreVertical,
} from '@wordpress/icons';
import { DropdownMenu, MenuItem, MenuGroup } from '@wordpress/components';
import { useDispatch, useSelect } from '@wordpress/data';
import { __, sprintf } from '@wordpress/i18n';
import { BlockTitle, store as blockEditorStore } from '@wordpress/block-editor';

const POPOVER_PROPS = {
className: 'block-editor-block-settings-menu__popover',
Copy link
Contributor

@ciampo ciampo Jun 13, 2023

Choose a reason for hiding this comment

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

The styles associated with this class name belong to the block-editor package.

We should avoid re-using class names (especially across packages) as it can lead to unexpected side effects. I suggest we add a new classname specific to this file/package, and (if needed) copy the styles from the block editor over.

The same should be done for the other "leaf more dropdown" component

Copy link
Contributor

Choose a reason for hiding this comment

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

After looking more into this, it doesn't look like this class achieved much in terms of styling (the padding override doesn't seem to have effect). I'll also look into other parts of the repo relying on this, and will potentially remove the class altogether

position: 'bottom right',
variant: 'toolbar',
};

const BLOCKS_THAT_CAN_BE_CONVERTED_TO_SUBMENU = [
'core/navigation-link',
'core/navigation-submenu',
];

function AddSubmenuItem( { block, onClose, expandedState, expand } ) {
const { insertBlock, replaceBlock, replaceInnerBlocks } =
useDispatch( blockEditorStore );

const clientId = block.clientId;
const isDisabled = ! BLOCKS_THAT_CAN_BE_CONVERTED_TO_SUBMENU.includes(
block.name
);
return (
<MenuItem
icon={ addSubmenu }
disabled={ isDisabled }
onClick={ () => {
const updateSelectionOnInsert = false;
const newLink = createBlock( 'core/navigation-link' );

if ( block.name === 'core/navigation-submenu' ) {
insertBlock(
newLink,
block.innerBlocks.length,
clientId,
updateSelectionOnInsert
);
} else {
// Convert to a submenu if the block currently isn't one.
const newSubmenu = createBlock(
'core/navigation-submenu',
block.attributes,
block.innerBlocks
);

// The following must happen as two independent actions.
// Why? Because the offcanvas editor relies on the getLastInsertedBlocksClientIds
// selector to determine which block is "active". As the UX needs the newLink to be
// the "active" block it must be the last block to be inserted.
// Therefore the Submenu is first created and **then** the newLink is inserted
// thus ensuring it is the last inserted block.
replaceBlock( clientId, newSubmenu );

replaceInnerBlocks(
newSubmenu.clientId,
[ newLink ],
updateSelectionOnInsert
);
}
if ( ! expandedState[ block.clientId ] ) {
expand( block.clientId );
}
onClose();
} }
>
{ __( 'Add submenu link' ) }
</MenuItem>
);
}

export default function LeafMoreMenu( props ) {
const { block } = props;
const { clientId } = block;
const { moveBlocksDown, moveBlocksUp, removeBlocks } =
useDispatch( blockEditorStore );

const removeLabel = sprintf(
/* translators: %s: block name */
__( 'Remove %s' ),
BlockTitle( { clientId, maximumLength: 25 } )
);

const rootClientId = useSelect(
( select ) => {
const { getBlockRootClientId } = select( blockEditorStore );

return getBlockRootClientId( clientId );
},
[ clientId ]
);

return (
<DropdownMenu
icon={ moreVertical }
label={ __( 'Options' ) }
className="block-editor-block-settings-menu"
popoverProps={ POPOVER_PROPS }
noIcons
{ ...props }
Copy link
Contributor

Choose a reason for hiding this comment

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

There are quite a few properties that are being passed to DropdownMenu via { ...props } that are not relevant for the component (ie. blocks, expanded, expandedState, clientIds, ...).

This pattern has the potential to causing unwanted side effects, especially hard to track when performing wide refactors. We should be trying to restructure props more explicitly, especially when we know exactly where those props need to be used / forwarded.

>
{ ( { onClose } ) => (
<>
<MenuGroup>
<MenuItem
icon={ chevronUp }
onClick={ () => {
moveBlocksUp( [ clientId ], rootClientId );
onClose();
} }
>
{ __( 'Move up' ) }
</MenuItem>
<MenuItem
icon={ chevronDown }
onClick={ () => {
moveBlocksDown( [ clientId ], rootClientId );
onClose();
} }
>
{ __( 'Move down' ) }
</MenuItem>
<AddSubmenuItem
block={ block }
onClose={ onClose }
expanded
expandedState={ props.expandedState }
expand={ props.expand }
/>
</MenuGroup>
<MenuGroup>
<MenuItem
onClick={ () => {
removeBlocks( [ clientId ], false );
onClose();
} }
>
{ removeLabel }
</MenuItem>
</MenuGroup>
</>
) }
</DropdownMenu>
);
}
Original file line number Diff line number Diff line change
Expand Up @@ -19,8 +19,7 @@ import { store as coreStore } from '@wordpress/core-data';
* Internal dependencies
*/
import { unlock } from '../../private-apis';

const { PrivateListView, LeafMoreMenu } = unlock( blockEditorPrivateApis );
import LeafMoreMenu from './leaf-more-menu';

function CustomLinkAdditionalBlockUI( { block, onClose } ) {
const { updateBlockAttributes } = useDispatch( blockEditorStore );
Expand Down Expand Up @@ -145,6 +144,7 @@ export default function NavigationMenuContent( { rootClientId, onSelect } ) {
};
}, [ shouldKeepLoading, clientIdsTree, isLoading ] );

const { PrivateListView } = unlock( blockEditorPrivateApis );
const offCanvasOnselect = useCallback(
( block ) => {
if (
Expand Down