From 41e1ca829be98fd04d1fc859447b1ec769790b66 Mon Sep 17 00:00:00 2001 From: Aaron Robertshaw <60436221+aaronrobertshaw@users.noreply.github.com> Date: Tue, 14 Sep 2021 18:37:55 +1000 Subject: [PATCH] ToolsPanel: Refine component behaviour (#34530) * Separate default and optional controls in the menu * Set min-width on ToolsPanel dropdown menu * Make reset all a tertiary button as per design * Add aria-labels to tools panel menu items * Replace existing check and minus icons --- .../test/__snapshots__/control.js.snap | 2 +- .../test/__snapshots__/index.js.snap | 6 +- .../bottom-sheet/stepper-cell/stepper.ios.js | 4 +- .../src/tools-panel/stories/index.js | 2 + packages/components/src/tools-panel/styles.js | 4 + .../components/src/tools-panel/test/index.js | 30 ++++- .../tools-panel-header/component.js | 123 ++++++++++++++---- .../tools-panel/tools-panel-header/hook.js | 8 +- .../src/tools-panel/tools-panel-item/hook.js | 22 +++- .../src/tools-panel/tools-panel/hook.js | 54 ++++++-- packages/icons/CHANGELOG.md | 4 + packages/icons/src/index.js | 2 +- packages/icons/src/library/check.js | 2 +- packages/icons/src/library/minus.js | 12 -- packages/icons/src/library/reset.js | 12 ++ 15 files changed, 224 insertions(+), 63 deletions(-) delete mode 100644 packages/icons/src/library/minus.js create mode 100644 packages/icons/src/library/reset.js diff --git a/packages/block-editor/src/components/color-palette/test/__snapshots__/control.js.snap b/packages/block-editor/src/components/color-palette/test/__snapshots__/control.js.snap index 3d30e7664ae88..abf30378e091d 100644 --- a/packages/block-editor/src/components/color-palette/test/__snapshots__/control.js.snap +++ b/packages/block-editor/src/components/color-palette/test/__snapshots__/control.js.snap @@ -80,7 +80,7 @@ exports[`ColorPaletteControl matches the snapshot 1`] = ` xmlns="http://www.w3.org/2000/svg" > diff --git a/packages/components/src/color-palette/test/__snapshots__/index.js.snap b/packages/components/src/color-palette/test/__snapshots__/index.js.snap index 7bf8892b02e94..c75cd0d1cd013 100644 --- a/packages/components/src/color-palette/test/__snapshots__/index.js.snap +++ b/packages/components/src/color-palette/test/__snapshots__/index.js.snap @@ -365,7 +365,7 @@ exports[`ColorPalette should render a dynamic toolbar of colors 1`] = ` xmlns="http://www.w3.org/2000/svg" > } @@ -388,10 +388,10 @@ exports[`ColorPalette should render a dynamic toolbar of colors 1`] = ` xmlns="http://www.w3.org/2000/svg" > diff --git a/packages/components/src/mobile/bottom-sheet/stepper-cell/stepper.ios.js b/packages/components/src/mobile/bottom-sheet/stepper-cell/stepper.ios.js index 328f9496c4c0b..fa2318a9fd728 100644 --- a/packages/components/src/mobile/bottom-sheet/stepper-cell/stepper.ios.js +++ b/packages/components/src/mobile/bottom-sheet/stepper-cell/stepper.ios.js @@ -7,7 +7,7 @@ import { Text, TouchableOpacity, View } from 'react-native'; * WordPress dependencies */ import { withPreferredColorScheme } from '@wordpress/compose'; -import { Icon, minus, plus } from '@wordpress/icons'; +import { Icon, plus, reset } from '@wordpress/icons'; /** * Internal dependencies @@ -46,7 +46,7 @@ function Stepper( { onPressOut={ onPressOut } style={ [ buttonStyle, isMinValue ? { opacity: 0.4 } : null ] } > - + { hasValue={ () => !! width } label="Width" onDeselect={ () => setWidth( undefined ) } + isShownByDefault={ true } > { hasValue={ () => !! height } label="Height" onDeselect={ () => setHeight( undefined ) } + isShownByDefault={ true } > { expect( control ).toBeInTheDocument(); } ); - it( 'should prevent panel item rendering when toggled off via menu item', async () => { + it( 'should prevent optional panel item rendering when toggled off via menu item', async () => { renderPanel(); await selectMenuItem( controlProps.label ); const control = screen.queryByText( 'Example control' ); @@ -265,7 +265,7 @@ describe( 'ToolsPanel', () => { expect( control ).not.toBeInTheDocument(); } ); - it( 'should prevent shown by default item rendering when toggled off via menu item', async () => { + it( 'should continue to render shown by default item after it is toggled off via menu item', async () => { render( { await selectMenuItem( controlProps.label ); const resetControl = screen.queryByText( 'Default control' ); - expect( resetControl ).not.toBeInTheDocument(); + expect( resetControl ).toBeInTheDocument(); + } ); + + it( 'should render appropriate menu groups', async () => { + const { container } = render( + + +
Default control
+ + +
Optional control
+ + + ); + openDropdownMenu(); + + const menuGroups = container.querySelectorAll( + '.components-menu-group' + ); + + // Groups should be: default controls, optional controls & reset all. + expect( menuGroups.length ).toEqual( 3 ); } ); } ); diff --git a/packages/components/src/tools-panel/tools-panel-header/component.js b/packages/components/src/tools-panel/tools-panel-header/component.js index 0933106937849..2a1e82e2338f7 100644 --- a/packages/components/src/tools-panel/tools-panel-header/component.js +++ b/packages/components/src/tools-panel/tools-panel-header/component.js @@ -1,8 +1,8 @@ /** * WordPress dependencies */ -import { check, moreVertical } from '@wordpress/icons'; -import { __ } from '@wordpress/i18n'; +import { check, reset, moreVertical } from '@wordpress/icons'; +import { __, sprintf } from '@wordpress/i18n'; /** * Internal dependencies @@ -13,8 +13,87 @@ import MenuItem from '../../menu-item'; import { useToolsPanelHeader } from './hook'; import { contextConnect } from '../../ui/context'; +const DefaultControlsGroup = ( { items, onClose, toggleItem } ) => { + if ( ! items.length ) { + return null; + } + + return ( + + { items.map( ( [ label, hasValue ] ) => { + const icon = hasValue ? reset : check; + const itemLabel = hasValue + ? sprintf( + // translators: %s: The name of the control being reset e.g. "Padding". + __( 'Reset %s' ), + label + ) + : undefined; + + return ( + { + toggleItem( label ); + onClose(); + } } + role="menuitemcheckbox" + > + { label } + + ); + } ) } + + ); +}; + +const OptionalControlsGroup = ( { items, onClose, toggleItem } ) => { + if ( ! items.length ) { + return null; + } + + return ( + + { items.map( ( [ label, isSelected ] ) => { + const itemLabel = isSelected + ? sprintf( + // translators: %s: The name of the control being hidden and reset e.g. "Padding". + __( 'Hide and reset %s' ), + label + ) + : sprintf( + // translators: %s: The name of the control to display e.g. "Padding". + __( 'Show %s' ), + label + ); + + return ( + { + toggleItem( label ); + onClose(); + } } + role="menuitemcheckbox" + > + { label } + + ); + } ) } + + ); +}; + const ToolsPanelHeader = ( props, forwardedRef ) => { const { + dropdownMenuClassName, hasMenuItems, label: labelText, menuItems, @@ -27,35 +106,33 @@ const ToolsPanelHeader = ( props, forwardedRef ) => { return null; } + const defaultItems = Object.entries( menuItems?.default || {} ); + const optionalItems = Object.entries( menuItems?.optional || {} ); + return (

{ labelText } { hasMenuItems && ( - + { ( { onClose } ) => ( <> - - { Object.entries( menuItems ).map( - ( [ label, isSelected ] ) => { - return ( - { - toggleItem( label ); - onClose(); - } } - role="menuitemcheckbox" - > - { label } - - ); - } - ) } - + + { resetAll(); onClose(); diff --git a/packages/components/src/tools-panel/tools-panel-header/hook.js b/packages/components/src/tools-panel/tools-panel-header/hook.js index a3c2644ba7475..462d515b7b6fa 100644 --- a/packages/components/src/tools-panel/tools-panel-header/hook.js +++ b/packages/components/src/tools-panel/tools-panel-header/hook.js @@ -22,11 +22,15 @@ export function useToolsPanelHeader( props ) { return cx( styles.ToolsPanelHeader, className ); }, [ className ] ); - const { menuItems } = useToolsPanelContext(); - const hasMenuItems = !! Object.entries( menuItems ).length; + const dropdownMenuClassName = useMemo( () => { + return cx( styles.DropdownMenu ); + }, [] ); + + const { menuItems, hasMenuItems } = useToolsPanelContext(); return { ...otherProps, + dropdownMenuClassName, hasMenuItems, menuItems, className: classes, diff --git a/packages/components/src/tools-panel/tools-panel-item/hook.js b/packages/components/src/tools-panel/tools-panel-item/hook.js index 069e25ec26bc3..6b7b5cfd3023d 100644 --- a/packages/components/src/tools-panel/tools-panel-item/hook.js +++ b/packages/components/src/tools-panel/tools-panel-item/hook.js @@ -35,6 +35,7 @@ export function useToolsPanelItem( props ) { menuItems, registerPanelItem, deregisterPanelItem, + flagItemCustomization, isResetting, } = useToolsPanelContext(); @@ -55,10 +56,20 @@ export function useToolsPanelItem( props ) { }, [ panelId ] ); const isValueSet = hasValue(); + const wasValueSet = usePrevious( isValueSet ); + + // If this item represents a default control it will need to notify the + // panel when a custom value has been set. + useEffect( () => { + if ( isShownByDefault && isValueSet && ! wasValueSet ) { + flagItemCustomization( label ); + } + }, [ isValueSet, wasValueSet, isShownByDefault, label ] ); // Note: `label` is used as a key when building menu item state in // `ToolsPanel`. - const isMenuItemChecked = menuItems[ label ]; + const menuGroup = isShownByDefault ? 'default' : 'optional'; + const isMenuItemChecked = menuItems?.[ menuGroup ]?.[ label ]; const wasMenuItemChecked = usePrevious( isMenuItemChecked ); // Determine if the panel item's corresponding menu is being toggled and @@ -77,9 +88,16 @@ export function useToolsPanelItem( props ) { } }, [ isMenuItemChecked, wasMenuItemChecked, isValueSet, isResetting ] ); + // The item is shown if it is a default control regardless of whether it + // has a value. Optional items are shown when they are checked or have + // a value. + const isShown = isShownByDefault + ? menuItems?.[ menuGroup ]?.[ label ] !== undefined + : isMenuItemChecked; + return { ...otherProps, - isShown: isMenuItemChecked, + isShown, className: classes, }; } diff --git a/packages/components/src/tools-panel/tools-panel/hook.js b/packages/components/src/tools-panel/tools-panel/hook.js index 02e0d23f3f4a5..b2b7d35fc6bf5 100644 --- a/packages/components/src/tools-panel/tools-panel/hook.js +++ b/packages/components/src/tools-panel/tools-panel/hook.js @@ -10,6 +10,17 @@ import * as styles from '../styles'; import { useContextSystem } from '../../ui/context'; import { useCx } from '../../utils/hooks/use-cx'; +const generateMenuItems = ( { panelItems, reset } ) => { + const menuItems = { default: {}, optional: {} }; + + panelItems.forEach( ( { hasValue, isShownByDefault, label } ) => { + const group = isShownByDefault ? 'default' : 'optional'; + menuItems[ group ][ label ] = reset ? false : hasValue(); + } ); + + return menuItems; +}; + export function useToolsPanel( props ) { const { className, resetAll, panelId, ...otherProps } = useContextSystem( props, @@ -55,6 +66,20 @@ export function useToolsPanel( props ) { } }; + // Force a menu item to be checked. + // This is intended for use with default panel items. They are displayed + // separately to optional items and have different display states, + //.we need to update that when their value is customized. + const flagItemCustomization = ( label, group = 'default' ) => { + setMenuItems( { + ...menuItems, + [ group ]: { + ...menuItems[ group ], + [ label ]: true, + }, + } ); + }; + // Manage and share display state of menu items representing child controls. const [ menuItems, setMenuItems ] = useState( {} ); @@ -72,21 +97,27 @@ export function useToolsPanel( props ) { // Setup menuItems state as panel items register themselves. useEffect( () => { - const items = {}; - - panelItems.forEach( ( { hasValue, isShownByDefault, label } ) => { - items[ label ] = isShownByDefault || hasValue(); - } ); - + const items = generateMenuItems( { panelItems, reset: false } ); setMenuItems( items ); }, [ panelItems ] ); // Toggle the checked state of a menu item which is then used to determine // display of the item within the panel. const toggleItem = ( label ) => { + const currentItem = panelItems.find( ( item ) => item.label === label ); + + if ( ! currentItem ) { + return; + } + + const menuGroup = currentItem.isShownByDefault ? 'default' : 'optional'; + setMenuItems( { ...menuItems, - [ label ]: ! menuItems[ label ], + [ menuGroup ]: { + ...menuItems[ menuGroup ], + [ label ]: ! menuItems[ menuGroup ][ label ], + }, } ); }; @@ -98,12 +129,7 @@ export function useToolsPanel( props ) { } // Turn off display of all non-default items. - const resetMenuItems = {}; - - panelItems.forEach( ( { label, isShownByDefault } ) => { - resetMenuItems[ label ] = !! isShownByDefault; - } ); - + const resetMenuItems = generateMenuItems( { panelItems, reset: true } ); setMenuItems( resetMenuItems ); }; @@ -112,6 +138,8 @@ export function useToolsPanel( props ) { menuItems, registerPanelItem, deregisterPanelItem, + flagItemCustomization, + hasMenuItems: panelItems.length, isResetting: isResetting.current, }; diff --git a/packages/icons/CHANGELOG.md b/packages/icons/CHANGELOG.md index a841d3143a6a8..7102a054babc6 100644 --- a/packages/icons/CHANGELOG.md +++ b/packages/icons/CHANGELOG.md @@ -2,6 +2,10 @@ ## Unreleased +### Breaking Change + +- Removed the `minus` icon, which was only used once in the block editor, in favor of the new `reset` icon which offers a more refined vector. + ## 5.0.0 (2021-07-29) ### Breaking Change diff --git a/packages/icons/src/index.js b/packages/icons/src/index.js index bd12e43bd1a7f..a30d76f5c5486 100644 --- a/packages/icons/src/index.js +++ b/packages/icons/src/index.js @@ -120,7 +120,6 @@ export { default as media } from './library/media'; export { default as mediaAndText } from './library/media-and-text'; export { default as megaphone } from './library/megaphone'; export { default as menu } from './library/menu'; -export { default as minus } from './library/minus'; export { default as mobile } from './library/mobile'; export { default as more } from './library/more'; export { default as moreHorizontal } from './library/more-horizontal'; @@ -172,6 +171,7 @@ export { default as receipt } from './library/receipt'; export { default as redo } from './library/redo'; export { default as removeBug } from './library/remove-bug'; export { default as replace } from './library/replace'; +export { default as reset } from './library/reset'; export { default as resizeCornerNE } from './library/resize-corner-n-e'; export { default as reusableBlock } from './library/reusable-block'; export { default as symbol } from './library/symbol'; diff --git a/packages/icons/src/library/check.js b/packages/icons/src/library/check.js index c4cca5af773b3..c4419ebea67f5 100644 --- a/packages/icons/src/library/check.js +++ b/packages/icons/src/library/check.js @@ -5,7 +5,7 @@ import { SVG, Path } from '@wordpress/primitives'; const check = ( - + ); diff --git a/packages/icons/src/library/minus.js b/packages/icons/src/library/minus.js deleted file mode 100644 index 4f2f3653a9fe0..0000000000000 --- a/packages/icons/src/library/minus.js +++ /dev/null @@ -1,12 +0,0 @@ -/** - * WordPress dependencies - */ -import { SVG, Path } from '@wordpress/primitives'; - -const minus = ( - - - -); - -export default minus; diff --git a/packages/icons/src/library/reset.js b/packages/icons/src/library/reset.js new file mode 100644 index 0000000000000..05fc4b9385454 --- /dev/null +++ b/packages/icons/src/library/reset.js @@ -0,0 +1,12 @@ +/** + * WordPress dependencies + */ +import { Path, SVG } from '@wordpress/primitives'; + +const reset = ( + + + +); + +export default reset;