-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Changes from 10 commits
5879282
8b19926
7feea4b
910c5e5
530410b
d761f4c
b1dc7c8
30cdfaa
dedf53d
3a9be4b
0a4a92e
f4ea440
888c19a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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, | ||
] | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? 🙂 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I also think context saving some repeated props works. |
||
); | ||
|
||
return ( | ||
|
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; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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:
And the ellipsis menu is supposed to include everything from BlockSettings PLUS everything from the toolbar as well:
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 :-)
There was a problem hiding this comment.
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 theBlockSettings
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 exampleBlockNavigatorItemSettings
.Also, since
BlockSettings
has Slots we can use these to extend that in theBlockNavigatorItemSettings
. If we want slots to be available for actions/ items specific to toBlockNavigatorItem
then we'd add slots toBlockNavigatorItemSettings
.There was a problem hiding this comment.
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 #22463There was a problem hiding this comment.
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. TheNavigator
is calledNavigation
internally and theItem
is calledBlock
, so I reckonBlockNavigationBlockSettings
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
.There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.