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.

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

Deprecating isPressed in Button component #54740

Merged
1 change: 1 addition & 0 deletions packages/components/CHANGELOG.md
Expand Up @@ -5,6 +5,7 @@
### Enhancements

- `SearchControl`: polish metrics for `compact` size variant ([#54663](https://github.com/WordPress/gutenberg/pull/54663)).
- `Button`: deprecating `isPressed` prop in favour of `aria-pressed` ([#54740](https://github.com/WordPress/gutenberg/pull/54740)).

### Bug Fix

Expand Down
2 changes: 2 additions & 0 deletions packages/components/src/button/README.md
Expand Up @@ -186,6 +186,8 @@ Renders a red text-based button style to indicate destructive behavior.

Renders a pressed button style.

If the native `aria-pressed` attribute is also set, it will take precedence.

- Required: No

#### `isSmall`: `boolean`
Expand Down
37 changes: 30 additions & 7 deletions packages/components/src/button/index.tsx
Expand Up @@ -34,6 +34,7 @@ function useDeprecatedProps( {
isSecondary,
isTertiary,
isLink,
isPressed,
isSmall,
size,
variant,
Expand All @@ -42,6 +43,11 @@ function useDeprecatedProps( {
let computedSize = size;
let computedVariant = variant;

const newProps: { 'aria-pressed'?: boolean } = {
// @TODO Mark `isPressed` as deprecated
'aria-pressed': isPressed,
};

if ( isSmall ) {
computedSize ??= 'small';
}
Expand Down Expand Up @@ -73,6 +79,7 @@ function useDeprecatedProps( {
}

return {
...newProps,
...otherProps,
size: computedSize,
variant: computedVariant,
Expand All @@ -85,7 +92,6 @@ export function UnforwardedButton(
) {
const {
__next40pxDefaultSize,
isPressed,
isBusy,
isDestructive,
className,
Expand All @@ -106,10 +112,16 @@ export function UnforwardedButton(
...buttonOrAnchorProps
} = useDeprecatedProps( props );

const { href, target, ...additionalProps } =
'href' in buttonOrAnchorProps
? buttonOrAnchorProps
: { href: undefined, target: undefined, ...buttonOrAnchorProps };
const {
href,
target,
'aria-checked': ariaChecked,
'aria-pressed': ariaPressed,
'aria-selected': ariaSelected,
...additionalProps
} = 'href' in buttonOrAnchorProps
? buttonOrAnchorProps
: { href: undefined, target: undefined, ...buttonOrAnchorProps };

const instanceId = useInstanceId(
Button,
Expand All @@ -124,14 +136,23 @@ export function UnforwardedButton(
// Tooltip should not considered as a child
children?.[ 0 ]?.props?.className !== 'components-tooltip' );

const truthyAriaPressedValues: ( typeof ariaPressed )[] = [
true,
'true',
'mixed',
];

const classes = classnames( 'components-button', className, {
'is-next-40px-default-size': __next40pxDefaultSize,
'is-secondary': variant === 'secondary',
'is-primary': variant === 'primary',
'is-small': size === 'small',
'is-compact': size === 'compact',
'is-tertiary': variant === 'tertiary',
'is-pressed': isPressed,

'is-pressed': truthyAriaPressedValues.includes( ariaPressed ),
'is-pressed-mixed': ariaPressed === 'mixed',

'is-busy': isBusy,
'is-link': variant === 'link',
'is-destructive': isDestructive,
Expand All @@ -146,7 +167,9 @@ export function UnforwardedButton(
? {
type: 'button',
disabled: trulyDisabled,
'aria-pressed': isPressed,
'aria-checked': ariaChecked,
'aria-pressed': ariaPressed,
'aria-selected': ariaSelected,
}
: {};
const anchorProps: ComponentPropsWithoutRef< 'a' > =
Expand Down
11 changes: 11 additions & 0 deletions packages/components/src/button/stories/index.story.tsx
Expand Up @@ -26,6 +26,17 @@ const meta: Meta< typeof Button > = {
component: Button,
argTypes: {
// Overrides a limitation of the docgen interpreting our TS types for this as required.
'aria-pressed': {
control: { type: 'select' },
description:
'Indicates the current "pressed" state, implying it is a toggle button. Implicitly set by `isPressed`, but takes precedence if both are provided.',
options: [ undefined, 'true', 'false', 'mixed' ],
table: {
type: {
summary: 'boolean | "true" | "false" | "mixed"',
},
},
},
href: { type: { name: 'string', required: false } },
icon: {
control: { type: 'select' },
Expand Down
3 changes: 2 additions & 1 deletion packages/components/src/button/style.scss
Expand Up @@ -308,7 +308,8 @@
}

// Toggled style.
&.is-pressed {
&[aria-pressed="true"],
&[aria-pressed="mixed"] {
Comment on lines +311 to +312
Copy link
Contributor

Choose a reason for hiding this comment

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

After seeing #55004, now I think about it, we could keep using the is-pressed class to assign those styles. Afterall, now the is-pressed class is assigned when truthyAriaPressedValues.includes( ariaPressed )

color: $components-color-foreground-inverted;
background: $components-color-foreground;

Expand Down
73 changes: 67 additions & 6 deletions packages/components/src/button/test/index.tsx
Expand Up @@ -72,12 +72,6 @@ describe( 'Button', () => {
expect( button ).toHaveClass( 'is-link' );
} );

it( 'should render a button element with is-pressed without button class', () => {
render( <Button isPressed /> );

expect( screen.getByRole( 'button' ) ).toHaveClass( 'is-pressed' );
} );

it( 'should render a button element with has-text when children are passed', async () => {
const user = userEvent.setup();

Expand Down Expand Up @@ -347,6 +341,56 @@ describe( 'Button', () => {

await cleanupTooltip( user );
} );

describe( 'using `aria-pressed` prop', () => {
it( 'should render a button element with is-pressed when `true`', () => {
render( <Button aria-pressed /> );

expect( screen.getByRole( 'button' ) ).toHaveClass(
'is-pressed'
);
} );

it( 'should render a button element with is-pressed when `"true"`', () => {
render( <Button aria-pressed="true" /> );

expect( screen.getByRole( 'button' ) ).toHaveClass(
'is-pressed'
);
} );

it( 'should render a button element with is-pressed/is-pressed-mixed when `"mixed"`', () => {
render( <Button aria-pressed="mixed" /> );

expect( screen.getByRole( 'button' ) ).toHaveClass(
'is-pressed is-pressed-mixed'
);
} );

it( 'should render a button element without is-pressed when `undefined`', () => {
render( <Button aria-pressed={ undefined } /> );

expect( screen.getByRole( 'button' ) ).not.toHaveClass(
'is-pressed'
);
} );

it( 'should render a button element without is-pressed when `false`', () => {
render( <Button aria-pressed={ false } /> );

expect( screen.getByRole( 'button' ) ).not.toHaveClass(
'is-pressed'
);
} );

it( 'should render a button element without is-pressed when `"false"`', () => {
render( <Button aria-pressed="false" /> );

expect( screen.getByRole( 'button' ) ).not.toHaveClass(
'is-pressed'
);
} );
} );
} );

describe( 'with href property', () => {
Expand Down Expand Up @@ -434,6 +478,23 @@ describe( 'Button', () => {
);
expect( screen.getByRole( 'button' ) ).toHaveClass( 'is-compact' );
} );

it( 'should not break when the legacy isPressed prop is passed', () => {
render( <Button isPressed /> );

expect( screen.getByRole( 'button' ) ).toHaveAttribute(
'aria-pressed',
'true'
);
} );

it( 'should prioritize the `aria-pressed` prop over `isPressed`', () => {
render( <Button isPressed aria-pressed="mixed" /> );
expect( screen.getByRole( 'button' ) ).toHaveAttribute(
'aria-pressed',
'mixed'
);
} );
} );

describe( 'static typing', () => {
Expand Down