From 28798dd54dc9093d8e23f5dc16bc82d20b43b33d Mon Sep 17 00:00:00 2001 From: Marco Ciampini Date: Mon, 27 Sep 2021 18:03:07 +0200 Subject: [PATCH 1/7] Item: mark `isAction` as deprecated, use `onClick` to discriminate if it should render as `button` --- .../components/src/item-group/item/README.md | 5 ++-- .../components/src/item-group/item/hook.ts | 30 ++++++++++++++++--- packages/components/src/item-group/types.ts | 13 ++++---- 3 files changed, 35 insertions(+), 13 deletions(-) diff --git a/packages/components/src/item-group/item/README.md b/packages/components/src/item-group/item/README.md index dc5ac92fca4ba..093d9fc192f6d 100644 --- a/packages/components/src/item-group/item/README.md +++ b/packages/components/src/item-group/item/README.md @@ -29,12 +29,11 @@ function Example() { ## Props -### `isAction`: `boolean` +### `onClick`: `React.MouseEventHandler` -Renders the item as an interactive `button` element. +Even handler for processing `click` events. When defined, the `Item` component will render as a `button` (unless differently specified via the `as` prop). - Required: No -- Default: `false` ### `size`: `'small' | 'medium' | 'large'` diff --git a/packages/components/src/item-group/item/hook.ts b/packages/components/src/item-group/item/hook.ts index 2ff7574751098..b30f6da7acf98 100644 --- a/packages/components/src/item-group/item/hook.ts +++ b/packages/components/src/item-group/item/hook.ts @@ -13,26 +13,47 @@ import { useItemGroupContext } from '../context'; import { useCx } from '../../utils/hooks/use-cx'; import type { ItemProps } from '../types'; +function useDeprecatedProps( { + as, + isAction = false, + ...otherProps +}: WordPressComponentProps< ItemProps, 'div' > ) { + let computedAs = as; + + if ( isAction ) { + computedAs ??= 'button'; + } + + return { + ...otherProps, + as: computedAs, + }; +} + export function useItem( props: WordPressComponentProps< ItemProps, 'div' > ) { const { - isAction = false, as: asProp, className, + onClick, role = 'listitem', size: sizeProp, ...otherProps - } = useContextSystem( props, 'Item' ); + } = useContextSystem( useDeprecatedProps( props ), 'Item' ); const { spacedAround, size: contextSize } = useItemGroupContext(); const size = sizeProp || contextSize; - const as = ( asProp || isAction ? 'button' : 'div' ) as ElementType; + const as = + asProp || + ( ( typeof onClick !== 'undefined' + ? 'button' + : 'div' ) as ElementType ); const cx = useCx(); const classes = cx( - isAction && styles.unstyledButton, + as === 'button' && styles.unstyledButton, styles.itemSizes[ size ] || styles.itemSizes.medium, styles.item, spacedAround && styles.spacedAround, @@ -44,6 +65,7 @@ export function useItem( props: WordPressComponentProps< ItemProps, 'div' > ) { return { as, className: classes, + onClick, wrapperClassName, role, ...otherProps, diff --git a/packages/components/src/item-group/types.ts b/packages/components/src/item-group/types.ts index ed07be8a133f1..e00d9437b9f36 100644 --- a/packages/components/src/item-group/types.ts +++ b/packages/components/src/item-group/types.ts @@ -32,12 +32,6 @@ export interface ItemGroupProps { } export interface ItemProps { - /** - * Renders the item as an interactive `button` element. - * - * @default false - */ - isAction?: boolean; /** * Determines the amount of padding within the component. * @@ -48,6 +42,13 @@ export interface ItemProps { * The children elements. */ children: React.ReactNode; + /** + * Renders the item as an interactive `button` element. + * + * @default false + * @deprecated + */ + isAction?: boolean; } export type ItemGroupContext = { From ef3d41d3350f1b2cc28521af6cf866e70856379d Mon Sep 17 00:00:00 2001 From: Marco Ciampini Date: Mon, 27 Sep 2021 18:03:51 +0200 Subject: [PATCH 2/7] Update and add tests --- .../test/__snapshots__/index.js.snap | 22 ------- .../components/src/item-group/test/index.js | 65 ++++++++++++++++--- 2 files changed, 55 insertions(+), 32 deletions(-) diff --git a/packages/components/src/item-group/test/__snapshots__/index.js.snap b/packages/components/src/item-group/test/__snapshots__/index.js.snap index 4f96fd6f7e650..4e5ee1d89f53d 100644 --- a/packages/components/src/item-group/test/__snapshots__/index.js.snap +++ b/packages/components/src/item-group/test/__snapshots__/index.js.snap @@ -33,28 +33,6 @@ Snapshot Diff: `; -exports[`ItemGroup Item should render a button with the isAction prop is true 1`] = ` -Snapshot Diff: -- First value -+ Second value - -
--
- Code is poetry --
-+ -
-`; - exports[`ItemGroup Item should use different amounts of padding depending on the value of the size prop 1`] = ` Snapshot Diff: - First value diff --git a/packages/components/src/item-group/test/index.js b/packages/components/src/item-group/test/index.js index 3ae1eb78d4586..2247e11077446 100644 --- a/packages/components/src/item-group/test/index.js +++ b/packages/components/src/item-group/test/index.js @@ -1,7 +1,7 @@ /** * External dependencies */ -import { render } from '@testing-library/react'; +import { fireEvent, render, screen } from '@testing-library/react'; /** * Internal dependencies @@ -79,18 +79,36 @@ describe( 'ItemGroup', () => { } ); describe( 'Item', () => { - it( 'should render a button with the isAction prop is true', () => { - // By default, `isAction` is `false` - const { container: normalItem } = render( - Code is poetry - ); - const { container: actionItem } = render( - Code is poetry + it( 'should render as a `button` if the `onClick` handler is specified', () => { + const spy = jest.fn(); + render( Code is poetry ); + + const button = screen.getByRole( 'button' ); + + expect( button ).toBeInTheDocument(); + + fireEvent.click( button ); + + expect( spy ).toHaveBeenCalled(); + } ); + + it( 'should give priority to the `as` prop even if the `onClick` handler is specified', () => { + const spy = jest.fn(); + const { rerender } = render( + Code is poetry ); - expect( normalItem.firstChild ).toMatchDiffSnapshot( - actionItem.firstChild + expect( screen.getByRole( 'button' ) ).toBeInTheDocument(); + expect( screen.queryByRole( 'label' ) ).not.toBeInTheDocument(); + + rerender( + + Code is poetry + ); + + expect( screen.queryByRole( 'button' ) ).not.toBeInTheDocument(); + expect( screen.getByRole( 'link' ) ).toBeInTheDocument(); } ); it( 'should use different amounts of padding depending on the value of the size prop', () => { @@ -132,5 +150,32 @@ describe( 'ItemGroup', () => { largeSize.firstChild ); } ); + + it( 'should render a button with the isAction prop is true', () => { + // By default, `isAction` is `false` + const { rerender } = render( Code is poetry ); + + expect( screen.queryByRole( 'button' ) ).not.toBeInTheDocument(); + + rerender( Code is poetry ); + + expect( screen.getByRole( 'button' ) ).toBeInTheDocument(); + } ); + + it( 'should give priority to the `as` prop even if the isAction prop is true', () => { + const { rerender } = render( Code is poetry ); + + expect( screen.getByRole( 'button' ) ).toBeInTheDocument(); + expect( screen.queryByRole( 'label' ) ).not.toBeInTheDocument(); + + rerender( + + Code is poetry + + ); + + expect( screen.queryByRole( 'button' ) ).not.toBeInTheDocument(); + expect( screen.getByRole( 'link' ) ).toBeInTheDocument(); + } ); } ); } ); From 501d0c1baf63b6fc4576753a49c8c5104f2583a9 Mon Sep 17 00:00:00 2001 From: Marco Ciampini Date: Mon, 27 Sep 2021 18:04:25 +0200 Subject: [PATCH 3/7] Update stories --- .../src/item-group/stories/index.js | 30 +++++++------------ 1 file changed, 11 insertions(+), 19 deletions(-) diff --git a/packages/components/src/item-group/stories/index.js b/packages/components/src/item-group/stories/index.js index 30fb1a02150aa..6c0ad5a671ffd 100644 --- a/packages/components/src/item-group/stories/index.js +++ b/packages/components/src/item-group/stories/index.js @@ -56,7 +56,6 @@ export const _default = () => { }, PROP_UNSET ), - isAction: boolean( 'Item 1: isAction', true ), }; // Do not pass the `size` prop when its value is `undefined`. @@ -68,16 +67,14 @@ export const _default = () => { return ( - alert( 'WordPress.org' ) }> + Code is Poetry (no click handlers) + alert( 'WordPress.org' ) }> Code is Poetry — Click me! - alert( 'WordPress.org' ) }> + alert( 'WordPress.org' ) }> Code is Poetry — Click me! - alert( 'WordPress.org' ) }> - Code is Poetry — Click me! - - alert( 'WordPress.org' ) }> + alert( 'WordPress.org' ) }> Code is Poetry — Click me! @@ -90,16 +87,14 @@ export const dropdown = () => ( trigger={ } > - alert( 'WordPress.org' ) }> - Code is Poetry — Click me! - - alert( 'WordPress.org' ) }> + Code is Poetry (no click handlers) + alert( 'WordPress.org' ) }> Code is Poetry — Click me! - alert( 'WordPress.org' ) }> + alert( 'WordPress.org' ) }> Code is Poetry — Click me! - alert( 'WordPress.org' ) }> + alert( 'WordPress.org' ) }> Code is Poetry — Click me! @@ -141,7 +136,7 @@ export const complexLayouts = () => { return ( - alert( 'Color palette' ) }> + alert( 'Color palette' ) }> @@ -156,7 +151,7 @@ export const complexLayouts = () => { - alert( 'Single color setting' ) }> + alert( 'Single color setting' ) }> { - alert( 'Single typography setting' ) } - > + alert( 'Single typography setting' ) }> From e2454ee465c2ab190619a5a9a272499716980f33 Mon Sep 17 00:00:00 2001 From: Marco Ciampini Date: Mon, 27 Sep 2021 18:05:09 +0200 Subject: [PATCH 4/7] Remove `isAction` from Global Styles Sidebar --- .../src/components/sidebar/global-styles-sidebar.js | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/packages/edit-site/src/components/sidebar/global-styles-sidebar.js b/packages/edit-site/src/components/sidebar/global-styles-sidebar.js index 2d740ec9a8f3e..5bd826922f1ce 100644 --- a/packages/edit-site/src/components/sidebar/global-styles-sidebar.js +++ b/packages/edit-site/src/components/sidebar/global-styles-sidebar.js @@ -206,11 +206,7 @@ function NavigationButton( { } ) { const navigator = useNavigator(); return ( - navigator.push( path, { isBack } ) } - { ...props } - > + navigator.push( path, { isBack } ) } { ...props }> { icon && ( From a148ceaa6384b1ee34d92512c369aa5e897dc7fa Mon Sep 17 00:00:00 2001 From: Marco Ciampini Date: Tue, 28 Sep 2021 09:57:57 +0200 Subject: [PATCH 5/7] Remove the `isAction` prop --- .../components/src/item-group/item/hook.ts | 19 +------------ .../components/src/item-group/test/index.js | 27 ------------------- packages/components/src/item-group/types.ts | 7 ----- 3 files changed, 1 insertion(+), 52 deletions(-) diff --git a/packages/components/src/item-group/item/hook.ts b/packages/components/src/item-group/item/hook.ts index b30f6da7acf98..bbe0abeaa53be 100644 --- a/packages/components/src/item-group/item/hook.ts +++ b/packages/components/src/item-group/item/hook.ts @@ -13,23 +13,6 @@ import { useItemGroupContext } from '../context'; import { useCx } from '../../utils/hooks/use-cx'; import type { ItemProps } from '../types'; -function useDeprecatedProps( { - as, - isAction = false, - ...otherProps -}: WordPressComponentProps< ItemProps, 'div' > ) { - let computedAs = as; - - if ( isAction ) { - computedAs ??= 'button'; - } - - return { - ...otherProps, - as: computedAs, - }; -} - export function useItem( props: WordPressComponentProps< ItemProps, 'div' > ) { const { as: asProp, @@ -38,7 +21,7 @@ export function useItem( props: WordPressComponentProps< ItemProps, 'div' > ) { role = 'listitem', size: sizeProp, ...otherProps - } = useContextSystem( useDeprecatedProps( props ), 'Item' ); + } = useContextSystem( props, 'Item' ); const { spacedAround, size: contextSize } = useItemGroupContext(); diff --git a/packages/components/src/item-group/test/index.js b/packages/components/src/item-group/test/index.js index 2247e11077446..739a02e74435b 100644 --- a/packages/components/src/item-group/test/index.js +++ b/packages/components/src/item-group/test/index.js @@ -150,32 +150,5 @@ describe( 'ItemGroup', () => { largeSize.firstChild ); } ); - - it( 'should render a button with the isAction prop is true', () => { - // By default, `isAction` is `false` - const { rerender } = render( Code is poetry ); - - expect( screen.queryByRole( 'button' ) ).not.toBeInTheDocument(); - - rerender( Code is poetry ); - - expect( screen.getByRole( 'button' ) ).toBeInTheDocument(); - } ); - - it( 'should give priority to the `as` prop even if the isAction prop is true', () => { - const { rerender } = render( Code is poetry ); - - expect( screen.getByRole( 'button' ) ).toBeInTheDocument(); - expect( screen.queryByRole( 'label' ) ).not.toBeInTheDocument(); - - rerender( - - Code is poetry - - ); - - expect( screen.queryByRole( 'button' ) ).not.toBeInTheDocument(); - expect( screen.getByRole( 'link' ) ).toBeInTheDocument(); - } ); } ); } ); diff --git a/packages/components/src/item-group/types.ts b/packages/components/src/item-group/types.ts index e00d9437b9f36..39aad8c6902c7 100644 --- a/packages/components/src/item-group/types.ts +++ b/packages/components/src/item-group/types.ts @@ -42,13 +42,6 @@ export interface ItemProps { * The children elements. */ children: React.ReactNode; - /** - * Renders the item as an interactive `button` element. - * - * @default false - * @deprecated - */ - isAction?: boolean; } export type ItemGroupContext = { From eb50c94db9addc4934945da512c66a36b702fbc9 Mon Sep 17 00:00:00 2001 From: Marco Ciampini Date: Tue, 28 Sep 2021 10:00:55 +0200 Subject: [PATCH 6/7] CHANGELOG --- packages/components/CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/packages/components/CHANGELOG.md b/packages/components/CHANGELOG.md index df9fef03d219b..c743ebb3dc8a5 100644 --- a/packages/components/CHANGELOG.md +++ b/packages/components/CHANGELOG.md @@ -7,6 +7,7 @@ - Removed the deprecated `position` and `menuLabel` from the `DropdownMenu` component ([#34537](https://github.com/WordPress/gutenberg/pull/34537)). - Removed the deprecated `onClickOutside` prop from the `Popover` component ([#34537](https://github.com/WordPress/gutenberg/pull/34537)). - Changed `RangeControl` component to not apply `shiftStep` to inputs from its `` ([35020](https://github.com/WordPress/gutenberg/pull/35020)). +- Removed `isAction` prop from `Item`. The component will now rely on `onClick` to render as a `button` ([35152](https://github.com/WordPress/gutenberg/pull/35152)). ### New Feature From c80a971520cca446766ce40062f17830176f4f8b Mon Sep 17 00:00:00 2001 From: Marco Ciampini Date: Tue, 28 Sep 2021 10:07:13 +0200 Subject: [PATCH 7/7] Memo-ise item s classname computation --- .../components/src/item-group/item/hook.ts | 21 +++++++++++++------ 1 file changed, 15 insertions(+), 6 deletions(-) diff --git a/packages/components/src/item-group/item/hook.ts b/packages/components/src/item-group/item/hook.ts index bbe0abeaa53be..d842a29b1eb03 100644 --- a/packages/components/src/item-group/item/hook.ts +++ b/packages/components/src/item-group/item/hook.ts @@ -4,6 +4,11 @@ // eslint-disable-next-line no-restricted-imports import type { ElementType } from 'react'; +/** + * WordPress dependencies + */ +import { useMemo } from '@wordpress/element'; + /** * Internal dependencies */ @@ -35,12 +40,16 @@ export function useItem( props: WordPressComponentProps< ItemProps, 'div' > ) { const cx = useCx(); - const classes = cx( - as === 'button' && styles.unstyledButton, - styles.itemSizes[ size ] || styles.itemSizes.medium, - styles.item, - spacedAround && styles.spacedAround, - className + const classes = useMemo( + () => + cx( + as === 'button' && styles.unstyledButton, + styles.itemSizes[ size ] || styles.itemSizes.medium, + styles.item, + spacedAround && styles.spacedAround, + className + ), + [ as, className, size, spacedAround ] ); const wrapperClassName = cx( styles.itemWrapper );