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.

Sign up for GitHub

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

Jump to bottom

Button: Introduce size prop #51842

Merged
merged 12 commits into from
Jun 23, 2023
Merged

Button: Introduce size prop #51842

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
b992cbe
Revert "Button: Add opt-in prop for larger `isSmall` size (#51012)"
mirka Jun 22, 2023
5f1f4a8
Add docs for `size` prop
mirka Jun 22, 2023
82cff38
Add CSS
mirka Jun 23, 2023
5446c80
Fixup
mirka Jun 23, 2023
9f703d6
Add TODO for deprecation
mirka Jun 23, 2023
ae12f01
Add test for back compat
mirka Jun 23, 2023
400a02b
Fixup
mirka Jun 23, 2023
ccbd9da
Add changelog
mirka Jun 23, 2023
47bc0ea
Tweak description for "compact"
mirka Jun 23, 2023
d03ba97
Note that the `size` prop takes precedence
mirka Jun 23, 2023
9ec488a
Add test for prop priority
mirka Jun 23, 2023
590964c
Stop leaking `spinButtonSize` prop for styling
mirka Jun 23, 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
2 changes: 1 addition & 1 deletion packages/base-styles/_variables.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ $icon-size: 24px;
$button-size: 36px;
$button-size-next-default-40px: 40px; // transitionary variable for next default button size
$button-size-small: 24px;
$button-size-small-next-default-32px: 32px; // transitionary variable for next small button size
$button-size-compact: 32px;
$header-height: 60px;
$panel-header-height: $grid-unit-60;
$nav-sidebar-width: 360px;
Expand Down
12 changes: 12 additions & 0 deletions packages/components/src/button/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -218,6 +218,18 @@ If provided, renders a [Tooltip](/packages/components/src/tooltip/README.md) com

- Required: No

#### `size`: `'default'` | `'compact'` | `'small'`

The size of the button.

- `'default'`: For normal text-label buttons, unless it is a toggle button.
- `'compact'`: For toggle buttons and icon buttons.
- `'small'`: For icon buttons associated with more advanced or auxiliary features.
Copy link
Member Author

Choose a reason for hiding this comment

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

@jasmussen Do these guidelines look ok?

Copy link
Contributor

Choose a reason for hiding this comment

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

This part of the sheet visually explains the difference between 40 and 32:

Screenshot 2023-06-23 at 14 42 39

Specifically, 40px is the default to use.

32px, and compact is a good word for it, is the one to use in certain situations, notably in the case of mixing toggle buttons with buttons.

So, we could tweak "compact":

  • 'compact': For toggle buttons, icon buttons, and buttons when used in context of either.

What do you think?



- Required: No
- Default: `'default'`

#### `target`: `string`

If provided with `href`, sets the `target` attribute to the `a`.
Expand Down
15 changes: 11 additions & 4 deletions packages/components/src/button/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,11 +33,18 @@ function useDeprecatedProps( {
isSecondary,
isTertiary,
isLink,
isSmall,
size,
variant,
...otherProps
}: ButtonProps & DeprecatedButtonProps ): ButtonProps {
let computedSize = size;
let computedVariant = variant;

if ( isSmall ) {
computedSize ??= 'small';
}

if ( isPrimary ) {
computedVariant ??= 'primary';
}
Expand Down Expand Up @@ -66,6 +73,7 @@ function useDeprecatedProps( {

return {
...otherProps,
size: computedSize,
variant: computedVariant,
};
}
Expand All @@ -76,8 +84,6 @@ export function UnforwardedButton(
) {
const {
__next40pxDefaultSize,
__next32pxSmallSize,
isSmall,
isPressed,
isBusy,
isDestructive,
Expand All @@ -91,6 +97,7 @@ export function UnforwardedButton(
shortcut,
label,
children,
size = 'default',
text,
variant,
__experimentalIsFocusable: isFocusable,
Expand Down Expand Up @@ -118,10 +125,10 @@ export function UnforwardedButton(

const classes = classnames( 'components-button', className, {
'is-next-40px-default-size': __next40pxDefaultSize,
'is-next-32px-small-size': __next32pxSmallSize,
'is-secondary': variant === 'secondary',
'is-primary': variant === 'primary',
'is-small': isSmall,
'is-small': size === 'small',
'is-compact': size === 'compact',
'is-tertiary': variant === 'tertiary',
'is-pressed': isPressed,
'is-busy': isBusy,
Expand Down
25 changes: 13 additions & 12 deletions packages/components/src/button/style.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -257,25 +257,26 @@
/* stylelint-enable */
}

&.is-compact {
height: $button-size-compact;

&.has-icon:not(.has-text) {
padding: 0;
width: $button-size-compact;
min-width: $button-size-compact;
}
}

&.is-small {
height: $button-size-small-next-default-32px;
height: $button-size-small;
line-height: 22px;
padding: 0 8px;
font-size: 11px;

&.has-icon:not(.has-text) {
padding: 0;
width: $button-size-small-next-default-32px;
min-width: $button-size-small-next-default-32px;
}

&:not(.is-next-32px-small-size) {
height: $button-size-small;

&.has-icon:not(.has-text) {
width: $button-size-small;
min-width: $button-size-small;
}
width: $button-size-small;
min-width: $button-size-small;
}
}

Expand Down
5 changes: 5 additions & 0 deletions packages/components/src/button/test/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -402,6 +402,11 @@ describe( 'Button', () => {
);
expect( console ).toHaveWarned();
} );

it( 'should not break when the legacy isSmall prop is passed', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we also add a test about the interaction between isSmall and size, to make sure that they behave as expected in terms of which prop has priority?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good idea 9ec488a

render( <Button isSmall /> );
expect( screen.getByRole( 'button' ) ).toHaveClass( 'is-small' );
} );
} );

describe( 'static typing', () => {
Expand Down
24 changes: 15 additions & 9 deletions packages/components/src/button/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,15 +25,6 @@ type BaseButtonProps = {
* @default false
*/
__next40pxDefaultSize?: boolean;
/**
* Start opting into the larger `isSmall` button size that will become the
* default small size in a future version.
*
* Only takes effect when the `isSmall` prop is `true`.
*
* @default false
*/
__next32pxSmallSize?: boolean;
/**
* The button's children.
*/
Expand Down Expand Up @@ -74,8 +65,13 @@ type BaseButtonProps = {
* Renders a pressed button style.
*/
isPressed?: boolean;
// TODO: Deprecate officially (add console warning and move to DeprecatedButtonProps).
Copy link
Member Author

Choose a reason for hiding this comment

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

This can only be done after all the usages in the Gutenberg app are migrated.

/**
* Decreases the size of the button.
*
* Deprecated in favor of the `size` prop.
*
* @deprecated Use the `'small'` value on the `size` prop instead.
*/
isSmall?: boolean;
/**
Expand All @@ -92,6 +88,16 @@ type BaseButtonProps = {
* If provided, renders a Tooltip component for the button.
*/
showTooltip?: boolean;
/**
* The size of the button.
*
* - `'default'`: For normal text-label buttons, unless it is a toggle button.
* - `'compact'`: For toggle buttons and icon buttons.
* - `'small'`: For icon buttons associated with more advanced or auxiliary features.
*
* @default 'default'
Copy link
Contributor

Choose a reason for hiding this comment

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

We should probably add a note about the size prop having priority over the isSmall prop, when defined. That should probably be added to the description of both props.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes! d03ba97

*/
size?: 'default' | 'compact' | 'small';
/**
* If provided, displays the given text inside the button. If the button contains children elements, the text is displayed before them.
*/
Expand Down
14 changes: 9 additions & 5 deletions packages/components/src/font-size-picker/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ import { useState, useMemo, forwardRef } from '@wordpress/element';
/**
* Internal dependencies
*/
import { Button } from '../button';
import RangeControl from '../range-control';
import { Flex, FlexItem } from '../flex';
import {
Expand All @@ -31,7 +32,6 @@ import {
HeaderLabel,
HeaderToggle,
Controls,
ResetButton,
} from './styles';
import { Spacer } from '../spacer';
import FontSizePickerSelect from './font-size-picker-select';
Expand Down Expand Up @@ -268,17 +268,21 @@ const UnforwardedFontSizePicker = (
) }
{ withReset && (
<FlexItem>
<ResetButton
<Button
disabled={ value === undefined }
onClick={ () => {
onChange?.( undefined );
} }
isSmall
variant="secondary"
size={ size }
__next40pxDefaultSize
size={
size !== '__unstable-large'
? 'small'
: 'default'
}
>
{ __( 'Reset' ) }
</ResetButton>
</Button>
</FlexItem>
) }
</Flex>
Expand Down
10 changes: 0 additions & 10 deletions packages/components/src/font-size-picker/styles.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,6 @@ import Button from '../button';
import { HStack } from '../h-stack';
import { space } from '../ui/utils/space';
import { COLORS } from '../utils';
import type { FontSizePickerProps } from './types';

export const Container = styled.fieldset`
border: 0;
Expand Down Expand Up @@ -44,12 +43,3 @@ export const Controls = styled.div< {
${ ( props ) =>
! props.__nextHasNoMarginBottom && `margin-bottom: ${ space( 6 ) };` }
`;

export const ResetButton = styled( Button )< {
size: FontSizePickerProps[ 'size' ];
} >`
&&& {
height: ${ ( props ) =>
props.size === '__unstable-large' ? '40px' : '30px' };
}
Comment on lines -51 to -54
Copy link
Member Author

Choose a reason for hiding this comment

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

Not sure when it regressed, but these height overrides weren't currently working. The reset button was rendered as 24px no matter what.

In this PR, I went ahead and simplified it a bit so that the reset button is 40px in the __unstable-large case, and 24px otherwise. This should be ok because we'll be phasing out the non-large variants anyway.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds good. I wonder if compact would be a better fallback than small here, though? Although it doesn't seem to matter much, since we're phasing it out anyway, as you mentioned

Copy link
Member Author

Choose a reason for hiding this comment

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

I definitely considered it, but a 2px difference between the text field and the button would irritate a designer more than a 6px difference would 😆

`;
4 changes: 2 additions & 2 deletions packages/components/src/number-control/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -244,7 +244,7 @@ function UnforwardedNumberControl(
onClick={ buildSpinButtonClickHandler(
'up'
) }
size={ size }
spinButtonSize={ size }
Copy link
Member Author

Choose a reason for hiding this comment

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

Renamed to avoid prop name collision (no visual changes).

/>
<SpinButton
icon={ resetIcon }
Expand All @@ -255,7 +255,7 @@ function UnforwardedNumberControl(
onClick={ buildSpinButtonClickHandler(
'down'
) }
size={ size }
spinButtonSize={ size }
/>
</HStack>
</Spacer>
Expand Down
8 changes: 5 additions & 3 deletions packages/components/src/number-control/styles/number-control-styles.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,9 +36,11 @@ export const Input = styled( InputControl )`
`;

const spinButtonSizeStyles = ( {
size,
}: Pick< NumberControlProps, 'size' > ) => {
if ( size !== 'small' ) {
spinButtonSize,
}: {
spinButtonSize: NumberControlProps[ 'size' ];
} ) => {
if ( spinButtonSize !== 'small' ) {
return ``;
}

Expand Down