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

Ellipsis menu in block navigator #22427

Merged
merged 13 commits into from
May 21, 2020
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
24 changes: 23 additions & 1 deletion packages/block-editor/src/components/block-navigation/block.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ import classnames from 'classnames';
* WordPress dependencies
*/
import { __experimentalTreeGridCell as TreeGridCell } from '@wordpress/components';

import { moreVertical } from '@wordpress/icons';
import { useState } from '@wordpress/element';

/**
Expand All @@ -20,6 +20,8 @@ import {
} from '../block-mover/button';
import DescenderLines from './descender-lines';
import BlockNavigationBlockContents from './block-contents';
import BlockSettingsDropdown from '../block-settings-menu/block-settings-dropdown';
import { useBlockNavigationContext } from './context';

export default function BlockNavigationBlock( {
block,
Expand All @@ -45,6 +47,14 @@ export default function BlockNavigationBlock( {
'block-editor-block-navigation-block__mover-cell',
{ 'is-visible': hasVisibleMovers }
);
const {
__experimentalWithEllipsisMenu: withEllipsisMenu,
__experimentalWithEllipsisMenuMinLevel: ellipsisMenuMinLevel,
Comment on lines +51 to +52
Copy link
Contributor

Choose a reason for hiding this comment

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

Small thing, but I think we shouldn't use the term EllipsisMenu for the menu.

Elsewhere in the code it's called BlockSettings, so I think it'd be good to stay consistent with that.

Copy link
Contributor Author

@adamziel adamziel May 21, 2020

Choose a reason for hiding this comment

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

I have been pondering this exact issue. The two are not exactly the same. BlockSettings offers certain set of options:

Zrzut ekranu 2020-05-21 o 11 40 08

And the ellipsis menu is supposed to include everything from BlockSettings PLUS everything from the toolbar as well:

Zrzut ekranu 2020-05-21 o 11 40 47

While I don't love the name EllipsisMenu, I am also not convinced that using the name BlockSettings for both menus is the right thing to do here. Let's discuss that more on #22462 - we may need to rethink the naming strategy a little bit more as we also have BlockControls and then UniversalBlockControl. It's just too many similar names in circulation :-)

Copy link
Contributor

Choose a reason for hiding this comment

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

In either case EllipsisMenu is bad because it references the icon we use for the menu. What if we change that icon to a circle?

If we want a menu that includes the BlockControls and the BlockSettings which is used by a Block Navigator Item, we'd wrap the two (for reuse if that is the case) in a thing that references the functionality, for example BlockNavigatorItemSettings.

Also, since BlockSettings has Slots we can use these to extend that in the BlockNavigatorItemSettings. If we want slots to be available for actions/ items specific to to BlockNavigatorItem then we'd add slots to BlockNavigatorItemSettings.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@draganescu as I mentioned I agree we shouldn't call it EllipsisMenu - I was simply looking for a better option :-) BlockNavigatorItemSettings sounds pretty good so I went with it in #22463

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, I don't think BlockNavigatorItemSettings is right either. The Navigator is called Navigation internally and the Item is called Block, so I reckon BlockNavigationBlockSettings is perhaps the right way to go.

I know it's weirdly repetitive, but it's just a component name, and it makes it pretty clear where it lives and that it's a version of the BlockSettings.

Copy link
Member

Choose a reason for hiding this comment

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

@adamziel @draganescu @talldan I'm confused here, why shouldn't the two menus be the same?

Copy link
Contributor

Choose a reason for hiding this comment

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

Because in the Navigator the Items might have additional options that don't make sense in the block's menu. I don't know if that is the case @mtias. One example is add submenu, but that should sit fine with the block as well, why not?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, they seem to serve conceptually the exact same purpose. If there are certain features that would not make sense on one or the other, we should probably handle it through configuration / context.

} = useBlockNavigationContext();
const ellipsisMenuClassName = classnames(
'block-editor-block-navigation-block__menu-cell',
{ 'is-visible': hasVisibleMovers }
);

return (
<BlockNavigationLeaf
Expand Down Expand Up @@ -105,6 +115,18 @@ export default function BlockNavigationBlock( {
</TreeGridCell>
</>
) }

{ withEllipsisMenu && level >= ellipsisMenuMinLevel && (
<TreeGridCell className={ ellipsisMenuClassName }>
{ ( props ) => (
<BlockSettingsDropdown
clientIds={ [ clientId ] }
icon={ moreVertical }
{ ...props }
/>
) }
</TreeGridCell>
) }
</BlockNavigationLeaf>
);
}
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,8 @@ import { createContext, useContext } from '@wordpress/element';

export const BlockNavigationContext = createContext( {
__experimentalWithBlockNavigationSlots: false,
__experimentalWithEllipsisMenu: false,
__experimentalWithEllipsisMenuMinLevel: 0,
} );

export const useBlockNavigationContext = () =>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,7 @@ function BlockNavigationDropdownToggle( { isEnabled, onToggle, isOpen } ) {
function BlockNavigationDropdown( {
isDisabled,
__experimentalWithBlockNavigationSlots,
__experimentalWithEllipsisMenu,
} ) {
const hasBlocks = useSelect(
( select ) => !! select( 'core/block-editor' ).getBlockCount(),
Expand All @@ -79,6 +80,9 @@ function BlockNavigationDropdown( {
__experimentalWithBlockNavigationSlots={
__experimentalWithBlockNavigationSlots
}
__experimentalWithEllipsisMenu={
__experimentalWithEllipsisMenu
}
/>
) }
/>
Expand Down
14 changes: 14 additions & 0 deletions packages/block-editor/src/components/block-navigation/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,8 @@ function BlockNavigation( {
selectedBlockClientId,
selectBlock,
__experimentalWithBlockNavigationSlots,
__experimentalWithEllipsisMenu,
__experimentalWithEllipsisMenuMinLevel,
} ) {
if ( ! rootBlocks || rootBlocks.length === 0 ) {
return null;
Expand All @@ -41,6 +43,12 @@ function BlockNavigation( {
blocks={ [ rootBlock ] }
selectedBlockClientId={ selectedBlockClientId }
selectBlock={ selectBlock }
__experimentalWithEllipsisMenu={
__experimentalWithEllipsisMenu
}
__experimentalWithEllipsisMenuMinLevel={
__experimentalWithEllipsisMenuMinLevel
}
__experimentalWithBlockNavigationSlots={
__experimentalWithBlockNavigationSlots
}
Expand All @@ -52,6 +60,12 @@ function BlockNavigation( {
blocks={ rootBlocks }
selectedBlockClientId={ selectedBlockClientId }
selectBlock={ selectBlock }
__experimentalWithEllipsisMenu={
__experimentalWithEllipsisMenu
}
__experimentalWithEllipsisMenuMinLevel={
__experimentalWithEllipsisMenuMinLevel
}
__experimentalWithBlockNavigationSlots={
__experimentalWithBlockNavigationSlots
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,7 @@ $tree-item-height: 36px;
border-radius: $border-width;
}

.block-editor-block-navigation-block__menu-cell,
.block-editor-block-navigation-block__mover-cell {
width: $button-size;
opacity: 0;
Expand All @@ -66,6 +67,13 @@ $tree-item-height: 36px;
opacity: 1;
@include edit-post__fade-in-animation;
}

&,
.components-button.has-icon {
width: 24px;
min-width: 24px;
padding: 0;
}
}

.block-editor-block-mover-button {
Expand Down
17 changes: 15 additions & 2 deletions packages/block-editor/src/components/block-navigation/tree.js
Original file line number Diff line number Diff line change
Expand Up @@ -21,11 +21,24 @@ import { BlockNavigationContext } from './context';
*/
export default function BlockNavigationTree( {
__experimentalWithBlockNavigationSlots,
__experimentalWithEllipsisMenu,
__experimentalWithEllipsisMenuMinLevel,
...props
} ) {
const contextValue = useMemo(
() => ( { __experimentalWithBlockNavigationSlots } ),
[ __experimentalWithBlockNavigationSlots ]
() => ( {
__experimentalWithBlockNavigationSlots,
__experimentalWithEllipsisMenu,
__experimentalWithEllipsisMenuMinLevel:
typeof __experimentalWithEllipsisMenuMinLevel === 'number'
? __experimentalWithEllipsisMenuMinLevel
: 0,
} ),
[
__experimentalWithBlockNavigationSlots,
__experimentalWithEllipsisMenu,
__experimentalWithEllipsisMenuMinLevel,
]
Copy link
Member

Choose a reason for hiding this comment

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

Why are we using context for these two settings? Would prop-passing be simpler? 🙂

Copy link
Contributor Author

@adamziel adamziel May 20, 2020

Choose a reason for hiding this comment

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

Normally I'd say it would be, but this is a quite entangled network of components with a recursive structure - we'd have to pass these two props through ~5 layers of components. I think the context makes it simpler in this case.

Copy link
Contributor

Choose a reason for hiding this comment

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

I also think context saving some repeated props works.

);

return (
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,14 +13,16 @@ const { Fill: BlockSettingsMenuControls, Slot } = createSlotFill(
'BlockSettingsMenuControls'
);

const BlockSettingsMenuControlsSlot = ( { fillProps } ) => {
const BlockSettingsMenuControlsSlot = ( { fillProps, clientIds = null } ) => {
const { selectedBlocks } = useSelect( ( select ) => {
const { getBlocksByClientId, getSelectedBlockClientIds } = select(
'core/block-editor'
);
const ids =
clientIds !== null ? clientIds : getSelectedBlockClientIds();
return {
selectedBlocks: map(
getBlocksByClientId( getSelectedBlockClientIds() ),
getBlocksByClientId( ids ),
( block ) => block.name
),
};
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,174 @@
/**
* External dependencies
*/
import { castArray, flow } from 'lodash';

/**
* WordPress dependencies
*/
import { __, _n } from '@wordpress/i18n';
import {
DropdownMenu,
MenuGroup,
MenuItem,
ClipboardButton,
} from '@wordpress/components';
import { useSelect } from '@wordpress/data';
import { moreHorizontal } from '@wordpress/icons';
import { useState } from '@wordpress/element';
import { serialize } from '@wordpress/blocks';

/**
* Internal dependencies
*/
import BlockActions from '../block-actions';
import BlockModeToggle from './block-mode-toggle';
import BlockHTMLConvertButton from './block-html-convert-button';
import BlockUnknownConvertButton from './block-unknown-convert-button';
import __experimentalBlockSettingsMenuFirstItem from './block-settings-menu-first-item';
import BlockSettingsMenuControls from '../block-settings-menu-controls';

const POPOVER_PROPS = {
className: 'block-editor-block-settings-menu__popover',
position: 'bottom right',
isAlternate: true,
};

export function BlockSettingsDropdown( { clientIds, ...props } ) {
const blockClientIds = castArray( clientIds );
const count = blockClientIds.length;
const firstBlockClientId = blockClientIds[ 0 ];

const shortcuts = useSelect( ( select ) => {
const { getShortcutRepresentation } = select(
'core/keyboard-shortcuts'
);
return {
duplicate: getShortcutRepresentation(
'core/block-editor/duplicate'
),
remove: getShortcutRepresentation( 'core/block-editor/remove' ),
insertAfter: getShortcutRepresentation(
'core/block-editor/insert-after'
),
insertBefore: getShortcutRepresentation(
'core/block-editor/insert-before'
),
};
}, [] );

const [ hasCopied, setHasCopied ] = useState();

return (
<BlockActions clientIds={ clientIds }>
{ ( {
canDuplicate,
canInsertDefaultBlock,
isLocked,
onDuplicate,
onInsertAfter,
onInsertBefore,
onRemove,
blocks,
} ) => (
<DropdownMenu
icon={ moreHorizontal }
label={ __( 'More options' ) }
className="block-editor-block-settings-menu"
popoverProps={ POPOVER_PROPS }
noIcons
{ ...props }
>
{ ( { onClose } ) => (
<>
<MenuGroup>
<__experimentalBlockSettingsMenuFirstItem.Slot
fillProps={ { onClose } }
/>
{ count === 1 && (
<BlockUnknownConvertButton
clientId={ firstBlockClientId }
/>
) }
{ count === 1 && (
<BlockHTMLConvertButton
clientId={ firstBlockClientId }
/>
) }
<ClipboardButton
text={ () => serialize( blocks ) }
role="menuitem"
className="components-menu-item__button"
onCopy={ () => {
setHasCopied( true );
} }
onFinishCopy={ () => setHasCopied( false ) }
>
{ hasCopied
? __( 'Copied!' )
: __( 'Copy' ) }
</ClipboardButton>
{ canDuplicate && (
<MenuItem
onClick={ flow( onClose, onDuplicate ) }
shortcut={ shortcuts.duplicate }
>
{ __( 'Duplicate' ) }
</MenuItem>
) }
{ canInsertDefaultBlock && (
<>
<MenuItem
onClick={ flow(
onClose,
onInsertBefore
) }
shortcut={ shortcuts.insertBefore }
>
{ __( 'Insert Before' ) }
</MenuItem>
<MenuItem
onClick={ flow(
onClose,
onInsertAfter
) }
shortcut={ shortcuts.insertAfter }
>
{ __( 'Insert After' ) }
</MenuItem>
</>
) }
{ count === 1 && (
<BlockModeToggle
clientId={ firstBlockClientId }
onToggle={ onClose }
/>
) }
</MenuGroup>
<BlockSettingsMenuControls.Slot
fillProps={ { onClose } }
clientIds={ clientIds }
/>
<MenuGroup>
{ ! isLocked && (
<MenuItem
onClick={ flow( onClose, onRemove ) }
shortcut={ shortcuts.remove }
>
{ _n(
'Remove Block',
'Remove Blocks',
count
) }
</MenuItem>
) }
</MenuGroup>
</>
) }
</DropdownMenu>
) }
</BlockActions>
);
}

export default BlockSettingsDropdown;
Loading