From b3048b16e3a0857b5b7abd300a46cd4c508a5775 Mon Sep 17 00:00:00 2001 From: Haz Date: Tue, 7 Jan 2020 13:22:39 -0300 Subject: [PATCH] Components: Improve ToolbarButton (#18931) * Initial refactor on ToolbarButton * Update storyshots * Update ToolbarButton styles * Update snapshots * Remove unnecessary styles for components-toolbar-button * Update Toolbar Button imports * Refactors --- packages/components/src/index.js | 1 + packages/components/src/index.native.js | 1 + .../accessible-toolbar-button-container.js | 26 -- ...essible-toolbar-button-container.native.js | 6 - .../components/src/toolbar-button/index.js | 74 ++-- .../components/src/toolbar-group/index.js | 8 +- .../toolbar-group/toolbar-group-collapsed.js | 24 +- .../toolbar-group-collapsed.native.js | 12 +- packages/components/src/toolbar-item/index.js | 36 ++ .../src/toolbar-item/index.native.js | 16 + .../components/src/toolbar/stories/index.js | 56 ++- packages/components/src/toolbar/test/index.js | 4 +- storybook/test/__snapshots__/index.js.snap | 350 +++++++++--------- 13 files changed, 311 insertions(+), 303 deletions(-) delete mode 100644 packages/components/src/toolbar-button/accessible-toolbar-button-container.js delete mode 100644 packages/components/src/toolbar-button/accessible-toolbar-button-container.native.js create mode 100644 packages/components/src/toolbar-item/index.js create mode 100644 packages/components/src/toolbar-item/index.native.js diff --git a/packages/components/src/index.js b/packages/components/src/index.js index cb12eeb436699..78a3891a6d569 100644 --- a/packages/components/src/index.js +++ b/packages/components/src/index.js @@ -73,6 +73,7 @@ export { default as ToggleControl } from './toggle-control'; export { default as Toolbar } from './toolbar'; export { default as ToolbarButton } from './toolbar-button'; export { default as ToolbarGroup } from './toolbar-group'; +export { default as __experimentalToolbarItem } from './toolbar-item'; export { default as Tooltip } from './tooltip'; export { default as TreeSelect } from './tree-select'; export { default as VisuallyHidden } from './visually-hidden'; diff --git a/packages/components/src/index.native.js b/packages/components/src/index.native.js index 7663cf7f8a74e..087a7acd65831 100644 --- a/packages/components/src/index.native.js +++ b/packages/components/src/index.native.js @@ -7,6 +7,7 @@ export { default as Dropdown } from './dropdown'; export { default as Toolbar } from './toolbar'; export { default as ToolbarButton } from './toolbar-button'; export { default as ToolbarGroup } from './toolbar-group'; +export { default as __experimentalToolbarItem } from './toolbar-item'; export { default as Icon } from './icon'; export { default as IconButton } from './button/deprecated'; export { default as Spinner } from './spinner'; diff --git a/packages/components/src/toolbar-button/accessible-toolbar-button-container.js b/packages/components/src/toolbar-button/accessible-toolbar-button-container.js deleted file mode 100644 index 5d026c4b83e8c..0000000000000 --- a/packages/components/src/toolbar-button/accessible-toolbar-button-container.js +++ /dev/null @@ -1,26 +0,0 @@ -/** - * External dependencies - */ -import { useToolbarItem } from 'reakit/Toolbar'; - -/** - * WordPress dependencies - */ -import { Children, cloneElement, useContext } from '@wordpress/element'; - -/** - * Internal dependencies - */ -import ToolbarContext from '../toolbar-context'; - -function AccessibleToolbarButtonContainer( props ) { - const accessibleToolbarState = useContext( ToolbarContext ); - const childButton = Children.only( props.children ); - - // https://reakit.io/docs/composition/#props-hooks - const itemHTMLProps = useToolbarItem( accessibleToolbarState, childButton.props ); - - return
{ cloneElement( childButton, itemHTMLProps ) }
; -} - -export default AccessibleToolbarButtonContainer; diff --git a/packages/components/src/toolbar-button/accessible-toolbar-button-container.native.js b/packages/components/src/toolbar-button/accessible-toolbar-button-container.native.js deleted file mode 100644 index 7fe5e6657bcd1..0000000000000 --- a/packages/components/src/toolbar-button/accessible-toolbar-button-container.native.js +++ /dev/null @@ -1,6 +0,0 @@ -/** - * Internal dependencies - */ -import ToolbarButtonContainer from './toolbar-button-container'; - -export default ToolbarButtonContainer; diff --git a/packages/components/src/toolbar-button/index.js b/packages/components/src/toolbar-button/index.js index 665e7bc49fd6c..5ffa9ad2f3953 100644 --- a/packages/components/src/toolbar-button/index.js +++ b/packages/components/src/toolbar-button/index.js @@ -12,63 +12,55 @@ import { useContext } from '@wordpress/element'; * Internal dependencies */ import Button from '../button'; +import ToolbarItem from '../toolbar-item'; import ToolbarContext from '../toolbar-context'; -import AccessibleToolbarButtonContainer from './accessible-toolbar-button-container'; import ToolbarButtonContainer from './toolbar-button-container'; function ToolbarButton( { containerClassName, - icon, - title, - shortcut, - subscript, - onClick, className, - isActive, - isDisabled, extraProps, children, + ...props } ) { - // It'll contain state if `ToolbarButton` is being used within - // `` const accessibleToolbarState = useContext( ToolbarContext ); - const button = ( - } + ); } diff --git a/packages/components/src/toolbar-group/index.js b/packages/components/src/toolbar-group/index.js index 866a671875c18..769df54be8af9 100644 --- a/packages/components/src/toolbar-group/index.js +++ b/packages/components/src/toolbar-group/index.js @@ -53,9 +53,8 @@ function ToolbarGroup( { children, className, isCollapsed, - icon, title, - ...otherProps + ...props } ) { // It'll contain state if `ToolbarGroup` is being used within // `` @@ -81,18 +80,17 @@ function ToolbarGroup( { if ( isCollapsed ) { return ( ); } return ( - + { flatMap( controlSets, ( controlSet, indexOfSet ) => controlSet.map( ( control, indexOfControl ) => ( ` const accessibleToolbarState = useContext( ToolbarContext ); @@ -28,22 +18,14 @@ function ToolbarGroupCollapsed( { const renderDropdownMenu = ( toggleProps ) => ( ); if ( accessibleToolbarState ) { - return ( - // https://reakit.io/docs/composition/#render-props - - { ( toolbarItemHTMLProps ) => renderDropdownMenu( toolbarItemHTMLProps ) } - - ); + return { renderDropdownMenu }; } return renderDropdownMenu(); diff --git a/packages/components/src/toolbar-group/toolbar-group-collapsed.native.js b/packages/components/src/toolbar-group/toolbar-group-collapsed.native.js index 718e1238792c4..b688d842ff596 100644 --- a/packages/components/src/toolbar-group/toolbar-group-collapsed.native.js +++ b/packages/components/src/toolbar-group/toolbar-group-collapsed.native.js @@ -3,16 +3,8 @@ */ import DropdownMenu from '../dropdown-menu'; -function ToolbarGroupCollapsed( { controls = [], className, icon, label } ) { - return ( - - ); +function ToolbarGroupCollapsed( { controls = [], ...props } ) { + return ; } export default ToolbarGroupCollapsed; diff --git a/packages/components/src/toolbar-item/index.js b/packages/components/src/toolbar-item/index.js new file mode 100644 index 0000000000000..00170174387bd --- /dev/null +++ b/packages/components/src/toolbar-item/index.js @@ -0,0 +1,36 @@ +/** + * External dependencies + */ +import { useToolbarItem } from 'reakit/Toolbar'; + +/** + * WordPress dependencies + */ +import { forwardRef, useContext } from '@wordpress/element'; + +/** + * Internal dependencies + */ +import ToolbarContext from '../toolbar-context'; + +function ToolbarItem( { children, ...props }, ref ) { + const accessibleToolbarState = useContext( ToolbarContext ); + // https://reakit.io/docs/composition/#props-hooks + const itemProps = useToolbarItem( accessibleToolbarState, { ...props, ref } ); + + if ( typeof children !== 'function' ) { + // eslint-disable-next-line no-console + console.warn( '`ToolbarItem` is a generic headless component that accepts only function children props' ); + return null; + } + + if ( ! accessibleToolbarState ) { + // eslint-disable-next-line no-console + console.warn( '`ToolbarItem` should be rendered within ``' ); + return null; + } + + return children( itemProps ); +} + +export default forwardRef( ToolbarItem ); diff --git a/packages/components/src/toolbar-item/index.native.js b/packages/components/src/toolbar-item/index.native.js new file mode 100644 index 0000000000000..9819d0ed02d83 --- /dev/null +++ b/packages/components/src/toolbar-item/index.native.js @@ -0,0 +1,16 @@ +/** + * WordPress dependencies + */ +import { forwardRef } from '@wordpress/element'; + +function ToolbarItem( { children, ...props }, ref ) { + if ( typeof children !== 'function' ) { + // eslint-disable-next-line no-console + console.warn( '`ToolbarItem` is a generic headless component that accepts only function children props' ); + return null; + } + + return children( { ...props, ref } ); +} + +export default forwardRef( ToolbarItem ); diff --git a/packages/components/src/toolbar/stories/index.js b/packages/components/src/toolbar/stories/index.js index 102f924f5a1a0..e817eca75e472 100644 --- a/packages/components/src/toolbar/stories/index.js +++ b/packages/components/src/toolbar/stories/index.js @@ -2,7 +2,14 @@ * Internal dependencies */ import Toolbar from '../'; -import { SVG, Path, ToolbarButton, ToolbarGroup } from '../../'; +import { + SVG, + Path, + ToolbarButton, + ToolbarGroup, + __experimentalToolbarItem as ToolbarItem, + DropdownMenu, +} from '../../'; export default { title: 'Components|Toolbar', component: Toolbar }; @@ -20,22 +27,30 @@ export const _default = () => { // id is required for server side rendering - + + + + + { ( toggleProps ) => ( + + ) } + - - - - + Text + + + { export const withoutGroup = () => { return ( // id is required for server side rendering - - - - + + + + ); }; diff --git a/packages/components/src/toolbar/test/index.js b/packages/components/src/toolbar/test/index.js index 2d454d1a8bff3..6a89be423647e 100644 --- a/packages/components/src/toolbar/test/index.js +++ b/packages/components/src/toolbar/test/index.js @@ -14,8 +14,8 @@ describe( 'Toolbar', () => { it( 'should render a toolbar with toolbar buttons', () => { const wrapper = mount( - - + + ); const control1 = wrapper.find( 'button[aria-label="control1"]' ); diff --git a/storybook/test/__snapshots__/index.js.snap b/storybook/test/__snapshots__/index.js.snap index d24d251f7f9c6..f972025eaf13b 100644 --- a/storybook/test/__snapshots__/index.js.snap +++ b/storybook/test/__snapshots__/index.js.snap @@ -3687,48 +3687,11 @@ exports[`Storyshots Components|Toolbar Default 1`] = ` >
-
- -
-
-
-
- -
-
- -
-
+
-
-
-
+ -
-
-`; - -exports[`Storyshots Components|Toolbar Without Group 1`] = ` -