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

Refactor Button component to TypeScript #46997

Merged
merged 46 commits into from
Jan 24, 2023
Merged
Show file tree
Hide file tree
Changes from 20 commits
Commits
Show all changes
46 commits
Select commit Hold shift + click to select a range
c05948f
Rename button/index.js to .jsx
kienstra Jan 3, 2023
5b57ef6
Possible approach to the prop types
kienstra Jan 5, 2023
718959c
Simplify the tag container
kienstra Jan 9, 2023
d8cae0c
Allow passing childrent to TagElement
kienstra Jan 9, 2023
d007837
Fix the type of onKeyDown
kienstra Jan 10, 2023
6ab8806
Rename test/index.js to .tsx, fix some errors
kienstra Jan 10, 2023
878c77d
Fix an eslint error from children being defined in the outer scope
kienstra Jan 10, 2023
78e0aaf
Rename stories/index.js to .tsx, add type annotations
kienstra Jan 10, 2023
00c05da
Merge branch 'trunk' into update/button-ts
kienstra Jan 10, 2023
9e6cc7d
Alphabetize types.ts types
kienstra Jan 10, 2023
9298bd7
Refactor TagElement to element
kienstra Jan 10, 2023
7314d0a
Alphabetize an import
kienstra Jan 10, 2023
0b616c7
Revert changes to element, this will fail type checking
kienstra Jan 10, 2023
3998f8b
Merge branch 'trunk' into update/button-ts
kienstra Jan 15, 2023
67efafe
Use the [] array type syntax
kienstra Jan 15, 2023
bbc7cd4
Improve the typing of hasChildren
kienstra Jan 15, 2023
0f7a242
Fix the typing of chilren.length
kienstra Jan 15, 2023
ca6ded1
Revert "Revert changes to element, this will fail type checking"
kienstra Jan 15, 2023
dadbb4b
Pass the generic of WordPressComponentProps a second argument
kienstra Jan 15, 2023
a447deb
Don't destructure out children, simply let them be in props
kienstra Jan 15, 2023
e702dd4
Use React.MousEvent
kienstra Jan 15, 2023
0badec8
Update README.md with types from types.ts
kienstra Jan 15, 2023
b9a3b50
Fix the type of shortcut
kienstra Jan 15, 2023
57866b7
Remove unnecessary Type annotations
kienstra Jan 15, 2023
d8dcaee
In README.md, alphabetize the props
kienstra Jan 15, 2023
5e270a1
Add a Required annotation to the variant prop
kienstra Jan 15, 2023
8ce4937
Add a JS DocBlock to Button
kienstra Jan 15, 2023
fd4144a
Add a CHANGELOG entry
kienstra Jan 15, 2023
916f9ac
Commit Lena's suggestion: Update packages/components/src/button/types.ts
kienstra Jan 21, 2023
3632a42
Commit Lena's suggestion: Update packages/components/src/button/depre…
kienstra Jan 21, 2023
309db5c
Commit Lena's suggestion: Update packages/components/src/button/index…
kienstra Jan 21, 2023
31cbdab
Apply Lena's suggestion to change 'div' to 'button'
kienstra Jan 22, 2023
2ca61e5
Remvoe needless type DisabledEvent
kienstra Jan 22, 2023
be1359d
Remove needless argTypes that Storybook will infer from types
kienstra Jan 22, 2023
35f8efb
Restore a deleted test, thanks to Lena's idea
kienstra Jan 22, 2023
b84337d
Don't expect the console to error
kienstra Jan 22, 2023
14e1137
Update tooltipPosition in README.md
kienstra Jan 22, 2023
12fa70f
Merge branch 'trunk' into update/button-ts
kienstra Jan 22, 2023
a38c275
Move the CHANGELOG entry to Unreleased
kienstra Jan 22, 2023
cea1f6b
Cherry-pick @mirka 's commit to make Button types specific to a or b…
kienstra Jan 23, 2023
ccd87bc
Commit Lena's static tests verbatim
kienstra Jan 23, 2023
561d6aa
Fix a failed test run that I caused
kienstra Jan 23, 2023
3892543
Rename CommonButtonProps to BaseButtonProps
kienstra Jan 23, 2023
01f3c9c
Merge in trunk, resolve conflict
kienstra Jan 23, 2023
07b7002
Commit Lena's suggestion: Update packages/components/src/button/stori…
kienstra Jan 23, 2023
dfef117
Commit Lena's diff for static typing test
kienstra Jan 24, 2023
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
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ const BorderBoxControlLinkedButton = (
<Tooltip text={ label }>
<View className={ className }>
<Button
{ ...buttonProps }
{ ...( buttonProps as Parameters< typeof Button >[ 0 ] ) }
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry to expand the scope outside of components/src/button/

This corrects a TS error:

ackages/components/src/border-box-control/border-box-control-linked-button/component.tsx:29:6 - error TS2322: Type '{ isSmall: true; icon: Element; iconSize: number; "aria-label": string; ref: ForwardedRef<any>; onClick: () => void; onChange?: FormEventHandler<HTMLDivElement> | undefined; ... 250 more ...; as?: keyof IntrinsicElements | undefined; }' is not assignable to type 'Omit<Pick<DetailedHTMLProps<AnchorHTMLAttributes<HTMLAnchorElement>, HTMLAnchorElement>, "key" | keyof AnchorHTMLAttributes<...>> | Pick<...>, "as" | keyof ButtonProps>'.
  Types of property 'onChange' are incompatible.
    Type 'FormEventHandler<HTMLDivElement> | undefined' is not assignable to type 'FormEventHandler<HTMLAnchorElement> | FormEventHandler<HTMLButtonElement> | undefined'.
      Type 'FormEventHandler<HTMLDivElement>' is not assignable to type 'FormEventHandler<HTMLAnchorElement> | FormEventHandler<HTMLButtonElement> | undefined'.
        Type 'FormEventHandler<HTMLDivElement>' is not assignable to type 'FormEventHandler<HTMLAnchorElement>'.
          Type 'HTMLDivElement' is missing the following properties from type 'HTMLAnchorElement': charset, coords, download, hreflang, and 21 more.

Copy link
Member

Choose a reason for hiding this comment

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

Not at all, you caught a legitimate typing error here 🙂

The error is coming from the fact that the props for BorderBoxControlLinkedButton (and also useBorderBoxControlLinkedButton) are typed as if the rest props would forward to a div, when in fact they forward to a button.

props: WordPressComponentProps< LinkedButtonProps, 'div' >,

☝️ So we should fix these to 'button' instead of casting buttonProps with as.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great idea, committed in 31cbdab

isSmall
icon={ isLinked ? link : linkOff }
iconSize={ 24 }
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -166,7 +166,7 @@ const BorderControlDropdown = (
onClick={ onToggle }
variant="tertiary"
aria-label={ toggleAriaLabel }
position={ dropdownPosition }
tooltipPosition={ dropdownPosition }
Copy link
Contributor Author

@kienstra kienstra Jan 15, 2023

Choose a reason for hiding this comment

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

<Button> has no position prop, so maybe this should be tooltipPosition.

This also has a showTooltip prop below.

label={ __( 'Border color and style picker' ) }
showTooltip={ true }
>
Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,8 @@
// @ts-nocheck
/**
* External dependencies
*/
import type { ForwardedRef } from 'react';

/**
* WordPress dependencies
*/
Expand All @@ -8,9 +12,23 @@ import { forwardRef } from '@wordpress/element';
/**
* Internal dependencies
*/
import Button from '../button';
import Button from '.';
import type { ButtonProps, DeprecatedIconButtonProps, TagName } from './types';
import type { WordPressComponentProps } from '../ui/context';

function IconButton( { labelPosition, size, tooltip, label, ...props }, ref ) {
function UnforwardedIconButton(
{
label,
labelPosition,
size,
tooltip,
...props
}: WordPressComponentProps<
ButtonProps & DeprecatedIconButtonProps,
TagName
>,
kienstra marked this conversation as resolved.
Show resolved Hide resolved
ref: ForwardedRef< any >
) {
deprecated( 'wp.components.IconButton', {
since: '5.4',
alternative: 'wp.components.Button',
Expand All @@ -29,4 +47,4 @@ function IconButton( { labelPosition, size, tooltip, label, ...props }, ref ) {
);
}

export default forwardRef( IconButton );
export default forwardRef( UnforwardedIconButton );
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
// @ts-nocheck
/**
* External dependencies
*/
import classnames from 'classnames';
import type { ForwardedRef, HTMLProps, MouseEvent, ReactElement } from 'react';

/**
* WordPress dependencies
Expand All @@ -17,8 +17,18 @@ import { useInstanceId } from '@wordpress/compose';
import Tooltip from '../tooltip';
import Icon from '../icon';
import { VisuallyHidden } from '../visually-hidden';

const disabledEventsOnDisabledButton = [ 'onMouseDown', 'onClick' ];
import type { WordPressComponentProps } from '../ui/context';
import type {
ButtonProps,
DeprecatedButtonProps,
DisabledEvent,
TagName,
} from './types';

const disabledEventsOnDisabledButton: DisabledEvent[] = [
'onMouseDown',
'onClick',
];
kienstra marked this conversation as resolved.
Show resolved Hide resolved

function useDeprecatedProps( {
isDefault,
Expand All @@ -28,7 +38,7 @@ function useDeprecatedProps( {
isLink,
variant,
...otherProps
} ) {
}: WordPressComponentProps< ButtonProps & DeprecatedButtonProps, TagName > ) {
let computedVariant = variant;

if ( isPrimary ) {
Expand Down Expand Up @@ -63,7 +73,10 @@ function useDeprecatedProps( {
};
}

export function Button( props, ref ) {
export function UnforwardedButton(
props: WordPressComponentProps< ButtonProps, TagName >,
ref: ForwardedRef< any >
) {
const {
href,
target,
Expand Down Expand Up @@ -93,10 +106,12 @@ export function Button( props, ref ) {
);

const hasChildren =
children?.[ 0 ] &&
children[ 0 ] !== null &&
// Tooltip should not considered as a child
children?.[ 0 ]?.props?.className !== 'components-tooltip';
( 'string' === typeof children && !! children ) ||
( Array.isArray( children ) &&
children?.[ 0 ] &&
children[ 0 ] !== null &&
// Tooltip should not considered as a child
children?.[ 0 ]?.props?.className !== 'components-tooltip' );
Copy link
Contributor Author

@kienstra kienstra Jan 15, 2023

Choose a reason for hiding this comment

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

This is more complex than I'd like.

But simply checking for Array.isArray( children ) before children?.[ 0 ] wasn't enough, and it caused unit tests to fail.

This is because children can be a string, which also has a .length property.


const classes = classnames( 'components-button', className, {
'is-secondary': variant === 'secondary',
Expand All @@ -113,7 +128,7 @@ export function Button( props, ref ) {

const trulyDisabled = disabled && ! isFocusable;
const Tag = href !== undefined && ! trulyDisabled ? 'a' : 'button';
const tagProps =
const tagProps: HTMLProps< TagName > =
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This typing looks unnecessary, but it prevents a TS error below.

Tag === 'a'
? { href, target }
: {
Expand All @@ -128,9 +143,11 @@ export function Button( props, ref ) {
tagProps[ 'aria-disabled' ] = true;

for ( const disabledEvent of disabledEventsOnDisabledButton ) {
additionalProps[ disabledEvent ] = ( event ) => {
event.stopPropagation();
event.preventDefault();
additionalProps[ disabledEvent ] = ( event: MouseEvent ) => {
if ( event ) {
event.stopPropagation();
event.preventDefault();
}
};
}
}
Expand All @@ -145,24 +162,26 @@ export function Button( props, ref ) {
// There's a label and...
( !! label &&
// The children are empty and...
! children?.length &&
! ( children as string | ReactElement[] )?.length &&
// The tooltip is not explicitly disabled.
false !== showTooltip ) );

const descriptionId = describedBy ? instanceId : null;
const descriptionId = describedBy ? instanceId : undefined;

const describedById =
additionalProps[ 'aria-describedby' ] || descriptionId;

const element = (
<Tag
{ ...tagProps }
{ ...additionalProps }
className={ classes }
aria-label={ additionalProps[ 'aria-label' ] || label }
aria-describedby={ describedById }
ref={ ref }
>
const elementProps = {
...tagProps,
...additionalProps,
className: classes,
'aria-label': additionalProps[ 'aria-label' ] || label,
'aria-describedby': describedById,
ref,
};

const elementChildren = (
<>
{ icon && iconPosition === 'left' && (
<Icon icon={ icon } size={ iconSize } />
) }
Expand All @@ -171,9 +190,30 @@ export function Button( props, ref ) {
<Icon icon={ icon } size={ iconSize } />
) }
{ children }
</Tag>
</>
);

const element =
Tag === 'a' ? (
<a
{ ...( elementProps as WordPressComponentProps<
ButtonProps,
'a'
> ) }
>
{ elementChildren }
</a>
) : (
<button
{ ...( elementProps as WordPressComponentProps<
ButtonProps,
'button'
> ) }
>
{ elementChildren }
</button>
);
Copy link
Contributor Author

@kienstra kienstra Jan 15, 2023

Choose a reason for hiding this comment

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

const element is really verbose.

But without this, there are type errors:

const Tag: "a" | "button"
Type '{ children: (ReactNode | Element)[]; className: string; "aria-label": string | undefined; "aria-describedby": string | undefined; ref: ForwardedRef<any>; ... 356 more ...; wrap?: string | undefined; }' is not assignable to type 'AnchorHTMLAttributes<HTMLAnchorElement>'.
  Types of property 'onCopy' are incompatible.
    Type 'ClipboardEventHandler<HTMLAnchorElement> | ClipboardEventHandler<HTMLButtonElement> | ClipboardEventHandler<...> | undefined' is not assignable to type 

Screen Shot 2023-01-15 at 4 25 28 PM


if ( ! shouldShowTooltip ) {
return (
<>
Expand All @@ -190,7 +230,12 @@ export function Button( props, ref ) {
return (
<>
<Tooltip
text={ children?.length && describedBy ? describedBy : label }
text={
( children as string | ReactElement[] )?.length &&
describedBy
? describedBy
: label
}
shortcut={ shortcut }
position={ tooltipPosition }
>
Expand All @@ -205,4 +250,5 @@ export function Button( props, ref ) {
);
}

export default forwardRef( Button );
export const Button = forwardRef( UnforwardedButton );
export default Button;
Original file line number Diff line number Diff line change
@@ -1,3 +1,9 @@
/**
* External dependencies
*/
import type { ComponentMeta, ComponentStory } from '@storybook/react';
import type { ReactNode } from 'react';

/**
* WordPress dependencies
*/
Expand All @@ -13,9 +19,9 @@ import {
* Internal dependencies
*/
import './style.css';
import Button from '../';
import Button from '..';

export default {
const meta: ComponentMeta< typeof Button > = {
title: 'Components/Button',
component: Button,
argTypes: {
mirka marked this conversation as resolved.
Show resolved Hide resolved
kienstra marked this conversation as resolved.
Show resolved Hide resolved
Expand Down Expand Up @@ -118,54 +124,57 @@ export default {
docs: { source: { state: 'open' } },
},
};
export default meta;

function Template( { children, ...props } ) {
return <Button { ...props }>{ children }</Button>;
}
const Template: ComponentStory< typeof Button > = ( props ) => {
return <Button { ...props }></Button>;
};

export const Default = Template.bind( {} );
export const Default: ComponentStory< typeof Button > = Template.bind( {} );
Default.args = {
children: 'Code is poetry',
};

export const Primary = Template.bind( {} );
export const Primary: ComponentStory< typeof Button > = Template.bind( {} );
Primary.args = {
...Default.args,
variant: 'primary',
};

export const Secondary = Template.bind( {} );
export const Secondary: ComponentStory< typeof Button > = Template.bind( {} );
Secondary.args = {
...Default.args,
variant: 'secondary',
};

export const Tertiary = Template.bind( {} );
export const Tertiary: ComponentStory< typeof Button > = Template.bind( {} );
Tertiary.args = {
...Default.args,
variant: 'tertiary',
};

export const Link = Template.bind( {} );
export const Link: ComponentStory< typeof Button > = Template.bind( {} );
Link.args = {
...Default.args,
variant: 'link',
};

export const IsDestructive = Template.bind( {} );
export const IsDestructive: ComponentStory< typeof Button > = Template.bind(
{}
);
IsDestructive.args = {
...Default.args,
isDestructive: true,
};

export const Icon = Template.bind( {} );
export const Icon: ComponentStory< typeof Button > = Template.bind( {} );
Icon.args = {
label: 'Code is poetry',
icon: 'wordpress',
};

export const groupedIcons = () => {
const GroupContainer = ( { children } ) => (
export const GroupedIcons: ComponentStory< typeof Button > = () => {
const GroupContainer = ( { children }: { children: ReactNode } ) => (
<div style={ { display: 'inline-flex' } }>{ children }</div>
);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,8 +12,8 @@ import { plusCircle } from '@wordpress/icons';
/**
* Internal dependencies
*/
import Button from '../';
import { Tooltip } from '../../';
import Button from '..';
import Tooltip from '../../tooltip';

jest.mock( '../../icon', () => () => <div data-testid="test-icon" /> );

Expand Down Expand Up @@ -139,16 +139,6 @@ describe( 'Button', () => {
expect( screen.getByRole( 'button' ) ).toHaveClass( 'gutenberg' );
} );

it( 'should render an additional WordPress prop of value awesome', () => {
render( <Button WordPress="awesome" /> );
mirka marked this conversation as resolved.
Show resolved Hide resolved

expect( console ).toHaveErrored();
expect( screen.getByRole( 'button' ) ).toHaveAttribute(
'wordpress',
'awesome'
);
} );

it( 'should render an icon button', () => {
render( <Button icon={ plusCircle } /> );
const button = screen.getByRole( 'button' );
Expand Down Expand Up @@ -360,28 +350,33 @@ describe( 'Button', () => {

describe( 'deprecated props', () => {
it( 'should not break when the legacy isPrimary prop is passed', () => {
// @ts-expect-error
render( <Button isPrimary /> );
mirka marked this conversation as resolved.
Show resolved Hide resolved
expect( screen.getByRole( 'button' ) ).toHaveClass( 'is-primary' );
} );

it( 'should not break when the legacy isSecondary prop is passed', () => {
// @ts-expect-error
render( <Button isSecondary /> );
expect( screen.getByRole( 'button' ) ).toHaveClass(
'is-secondary'
);
} );

it( 'should not break when the legacy isTertiary prop is passed', () => {
// @ts-expect-error
render( <Button isTertiary /> );
expect( screen.getByRole( 'button' ) ).toHaveClass( 'is-tertiary' );
} );

it( 'should not break when the legacy isLink prop is passed', () => {
// @ts-expect-error
render( <Button isLink /> );
expect( screen.getByRole( 'button' ) ).toHaveClass( 'is-link' );
} );

it( 'should warn when the isDefault prop is passed', () => {
// @ts-expect-error
render( <Button isDefault /> );
expect( screen.getByRole( 'button' ) ).toHaveClass(
'is-secondary'
Expand Down
Loading