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

Add icons to block settings dropdown #34785

Closed
wants to merge 2 commits into from
Closed

Add icons to block settings dropdown #34785

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
c161911
Add icons to block settings dropdown
ntsekouras Sep 13, 2021
555b203
narrow menu
ntsekouras Sep 14, 2021
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
6 changes: 5 additions & 1 deletion packages/block-editor/src/components/block-settings-menu/block-settings-dropdown.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ import { castArray, flow, noop } from 'lodash';
import { __ } from '@wordpress/i18n';
import { DropdownMenu, MenuGroup, MenuItem } from '@wordpress/components';
import { useSelect } from '@wordpress/data';
import { moreVertical } from '@wordpress/icons';
import { moreVertical, insertAfter, insertBefore } from '@wordpress/icons';

import { Children, cloneElement, useCallback } from '@wordpress/element';
import { serialize } from '@wordpress/blocks';
Expand Down Expand Up @@ -138,6 +138,8 @@ export function BlockSettingsDropdown( {
{ canInsertDefaultBlock && (
<>
<MenuItem
icon={ insertBefore }
iconPosition="left"
onClick={ flow(
onClose,
onInsertBefore
Expand All @@ -147,6 +149,8 @@ export function BlockSettingsDropdown( {
{ __( 'Insert before' ) }
</MenuItem>
<MenuItem
icon={ insertAfter }
iconPosition="left"
onClick={ flow(
onClose,
onInsertAfter
Expand Down
19 changes: 17 additions & 2 deletions packages/block-editor/src/components/block-settings-menu/style.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,18 @@
.block-editor-block-settings-menu__popover .components-dropdown-menu__menu {
padding: 0;
.block-editor-block-settings-menu__popover {
.components-dropdown-menu__menu {
padding: 0;
.components-menu-item__button {
padding-left: $grid-unit-50;
/**
* This is only applied to 'left' icons because 'right' icons
* are rendered inside the `button`.
*/
&.has-icon {
padding-left: $grid-unit;
}
.components-menu-item__item {
min-width: 120px;
}
}
}
}
5 changes: 5 additions & 0 deletions packages/block-editor/src/components/convert-to-group-buttons/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import { MenuItem } from '@wordpress/components';
import { _x } from '@wordpress/i18n';
import { switchToBlockType } from '@wordpress/blocks';
import { useDispatch } from '@wordpress/data';
import { group, ungroup } from '@wordpress/icons';

/**
* Internal dependencies
Expand Down Expand Up @@ -48,6 +49,8 @@ function ConvertToGroupButton( {
<>
{ isGroupable && (
<MenuItem
icon={ group }
iconPosition="left"
onClick={ () => {
onConvertToGroup();
onClose();
Expand All @@ -58,6 +61,8 @@ function ConvertToGroupButton( {
) }
{ isUngroupable && (
<MenuItem
icon={ ungroup }
iconPosition="left"
onClick={ () => {
onConvertFromGroup();
onClose();
Expand Down
8 changes: 8 additions & 0 deletions packages/components/src/menu-item/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,14 @@ Refer to documentation for [`label`](#label).

Refer to documentation for [Button's `icon` prop](/packages/components/src/icon-button/README.md#icon).

### `iconPosition`

- Type: `string`
- Required: No
- Default: `'right'`

Determines where to display the provided `icon`.

### `isSelected`

- Type: `boolean`
Expand Down
27 changes: 16 additions & 11 deletions packages/components/src/menu-item/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,15 +19,16 @@ import Icon from '../icon';
/**
* Renders a generic menu item for use inside the more menu.
*
* @param {Object} props Component props.
* @param {WPElement} props.children Element to render as child of button.
* @param {string} props.info Text to use as description for button text.
* @param {string} props.className Class to set on the container.
* @param {WPIcon} props.icon Button's `icon` prop.
* @param {string|Object} props.shortcut Shortcut's `shortcut` prop.
* @param {boolean} props.isSelected Whether or not the menu item is currently selected.
* @param {string} [props.role="menuitem"] ARIA role of the menu item.
* @param {Object} ref React Element ref.
* @param {Object} props Component props.
* @param {WPElement} props.children Element to render as child of button.
* @param {string} props.info Text to use as description for button text.
* @param {string} props.className Class to set on the container.
* @param {WPIcon} props.icon Button's `icon` prop.
* @param {string} [props.iconPosition="right"] Button's `icon` position (left|right).
* @param {string|Object} props.shortcut Shortcut's `shortcut` prop.
* @param {boolean} props.isSelected Whether or not the menu item is currently selected.
* @param {string} [props.role="menuitem"] ARIA role of the menu item.
* @param {Object} ref React Element ref.
*
* @return {WPComponent} The component to be rendered.
*/
Expand All @@ -37,6 +38,7 @@ export function MenuItem(
info,
className,
icon,
iconPosition = 'right',
shortcut,
isSelected,
role = 'menuitem',
Expand All @@ -57,7 +59,9 @@ export function MenuItem(

if ( icon && ! isString( icon ) ) {
icon = cloneElement( icon, {
className: 'components-menu-items__item-icon',
className: classnames( 'components-menu-items__item-icon', {
'has-icon-right': iconPosition === 'right',
} ),
} );
}

Expand All @@ -71,6 +75,7 @@ export function MenuItem(
: undefined
}
role={ role }
icon={ iconPosition === 'left' ? icon : undefined }
className={ className }
{ ...props }
>
Expand All @@ -79,7 +84,7 @@ export function MenuItem(
className="components-menu-item__shortcut"
shortcut={ shortcut }
/>
{ icon && <Icon icon={ icon } /> }
{ icon && iconPosition === 'right' && <Icon icon={ icon } /> }
Copy link
Contributor

@ciampo ciampo Sep 14, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

(same as #34710 (comment), since this PR includes the same changes to MenuItem)

This way of adding an icon to the right of the Button's content (as opposed to the left) feels hacky — have you considered adding this functionality directly on the Button's component?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

After discussing this a bit more with @ciampo in dm and exploring the code a bit, it seems that we have been hacking our way around for many current controls. It seems that last year I had added iconPosition in Button but used it with text added here.

Tool Selector uses a similar 'hack' by passing the icon as a label along with text.

So these usages and others that will surface by searching need to be consolidated through a more consistent API. We will probably need to make MenuItem always use the icon as a prop and also pass the iconPosition. This requires a greater effort and being extra cautious not to brake existing UIs and should be addressed in a separate PR.

</Button>
);
}
Expand Down
8 changes: 5 additions & 3 deletions packages/components/src/menu-item/style.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,13 +12,15 @@
}

.components-menu-items__item-icon {
margin-right: -2px; // This optically balances the icon.
margin-left: $grid-unit-30;
display: inline-block;
flex: 0 0 auto;
&.has-icon-right {
margin-right: -2px; // This optically balances the icon.
margin-left: $grid-unit-30;
}
}

.components-menu-item__shortcut + .components-menu-items__item-icon {
.components-menu-item__shortcut + .components-menu-items__item-icon.has-icon-right {
margin-left: $grid-unit-10;
}

Expand Down
4 changes: 2 additions & 2 deletions packages/components/src/menu-item/test/__snapshots__/index.js.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ exports[`MenuItem should match snapshot when all props provided 1`] = `
<Icon
icon={
<SVG
className="components-menu-items__item-icon"
className="components-menu-items__item-icon has-icon-right"
viewBox="0 0 24 24"
xmlns="http://www.w3.org/2000/svg"
>
Expand Down Expand Up @@ -79,7 +79,7 @@ exports[`MenuItem should match snapshot when isSelected and role are optionally
<Icon
icon={
<SVG
className="components-menu-items__item-icon"
className="components-menu-items__item-icon has-icon-right"
viewBox="0 0 24 24"
xmlns="http://www.w3.org/2000/svg"
>
Expand Down
2 changes: 2 additions & 0 deletions packages/edit-post/src/components/block-settings-menu/plugin-block-settings-menu-item.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,7 @@ const shouldRenderItem = ( selectedBlocks, allowedBlocks ) =>
const PluginBlockSettingsMenuItem = ( {
allowedBlocks,
icon,
iconPosition,
label,
onClick,
small,
Expand All @@ -100,6 +101,7 @@ const PluginBlockSettingsMenuItem = ( {
<MenuItem
onClick={ compose( onClick, onClose ) }
icon={ icon }
iconPosition={ iconPosition }
label={ small ? label : undefined }
role={ role }
>
Expand Down
1 change: 1 addition & 0 deletions ...eusable-blocks/src/components/reusable-blocks-menu-items/reusable-block-convert-button.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -114,6 +114,7 @@ export default function ReusableBlockConvertButton( {
<>
<MenuItem
icon={ reusableBlock }
iconPosition="left"
onClick={ () => {
setIsModalOpen( true );
} }
Expand Down