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

ToggleGroupControl: Add size variants #42008

Merged
merged 7 commits into from
Jul 14, 2022
Merged

ToggleGroupControl: Add size variants #42008

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
38b4537
Remove unused export
mirka Jun 28, 2022
e4a1085
ToggleGroupControl: Add size variants
mirka Jun 28, 2022
39750d4
Add stories
mirka Jun 28, 2022
3129622
Remove 'medium' hardcoded class name
mirka Jun 28, 2022
b52d7e3
Remove small size
mirka Jul 8, 2022
f8ba70c
Update snapshot
mirka Jul 8, 2022
45694d1
Add changelog
mirka Jul 8, 2022
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
4 changes: 4 additions & 0 deletions packages/components/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,10 @@

- `BoxControl`: Change ARIA role from `region` to `group` to avoid unwanted ARIA landmark regions ([#42094](https://github.com/WordPress/gutenberg/pull/42094)).

### Enhancement

- `ToggleGroupControl`: Add large size variant ([#42008](https://github.com/WordPress/gutenberg/pull/42008/)).

### Internal

- `Grid`: Convert to TypeScript ([#41923](https://github.com/WordPress/gutenberg/pull/41923)).
Expand Down
22 changes: 19 additions & 3 deletions packages/components/src/toggle-group-control/stories/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,12 @@ import Button from '../../button';
export default {
component: ToggleGroupControl,
title: 'Components (Experimental)/ToggleGroupControl',
argTypes: {
size: {
control: 'select',
options: [ 'default', '__unstable-large' ],
},
},
parameters: {
knobs: { disable: false },
},
Expand All @@ -33,7 +39,7 @@ const KNOBS_GROUPS = {
ToggleGroupControlOption: 'ToggleGroupControlOption',
};

const _default = ( { options } ) => {
const _default = ( { options, ...props } ) => {
const [ value, setValue ] = useState( options[ 0 ].value );
const label = text(
`${ KNOBS_GROUPS.ToggleGroupControl }: label`,
Expand Down Expand Up @@ -87,6 +93,7 @@ const _default = ( { options } ) => {
return (
<View>
<ToggleGroupControl
{ ...props }
onChange={ setValue }
value={ value }
label={ label }
Expand All @@ -109,6 +116,7 @@ Default.args = {
{ value: 'right', label: 'Right' },
{ value: 'justify', label: 'Justify' },
],
size: 'default',
};

export const WithTooltip = _default.bind( {} );
Expand All @@ -130,10 +138,11 @@ WithAriaLabel.args = {
],
};

export const WithIcons = () => {
export const WithIcons = ( props ) => {
const [ state, setState ] = useState();
return (
<ToggleGroupControl
{ ...props }
onChange={ setState }
value={ state }
label={ 'With icons' }
Expand All @@ -154,8 +163,11 @@ export const WithIcons = () => {
</ToggleGroupControl>
);
};
WithIcons.args = {
...Default.args,
};

export const WithReset = () => {
export const WithReset = ( props ) => {
const [ alignState, setAlignState ] = useState();
const aligns = [ 'Left', 'Center', 'Right' ];
const alignOptions = aligns.map( ( key, index ) => (
Expand All @@ -172,6 +184,7 @@ export const WithReset = () => {
return (
<View>
<ToggleGroupControl
{ ...props }
onChange={ setAlignState }
value={ alignState }
label={ 'Toggle Group Control' }
Expand All @@ -185,3 +198,6 @@ export const WithReset = () => {
</View>
);
};
WithReset.args = {
...Default.args,
};
8 changes: 4 additions & 4 deletions packages/components/src/toggle-group-control/test/__snapshots__/index.js.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,12 +28,12 @@ exports[`ToggleGroupControl should render correctly with icons 1`] = `
display: -webkit-inline-flex;
display: -ms-inline-flexbox;
display: inline-flex;
min-height: 36px;
min-width: 0;
padding: 2px;
position: relative;
-webkit-transition: -webkit-transform 100ms linear;
transition: transform 100ms linear;
min-height: 36px;
}

@media ( prefers-reduced-motion: reduce ) {
Expand Down Expand Up @@ -167,7 +167,7 @@ exports[`ToggleGroupControl should render correctly with icons 1`] = `
</div>
<div
aria-label="Test Toggle Group Control"
class="medium components-toggle-group-control emotion-6 emotion-7"
class="components-toggle-group-control emotion-6 emotion-7"
data-wp-c16t="true"
data-wp-component="ToggleGroupControl"
id="toggle-group-control-1"
Expand Down Expand Up @@ -281,12 +281,12 @@ exports[`ToggleGroupControl should render correctly with text options 1`] = `
display: -webkit-inline-flex;
display: -ms-inline-flexbox;
display: inline-flex;
min-height: 36px;
min-width: 0;
padding: 2px;
position: relative;
-webkit-transition: -webkit-transform 100ms linear;
transition: transform 100ms linear;
min-height: 36px;
}

@media ( prefers-reduced-motion: reduce ) {
Expand Down Expand Up @@ -393,7 +393,7 @@ exports[`ToggleGroupControl should render correctly with text options 1`] = `
</div>
<div
aria-label="Test Toggle Group Control"
class="medium components-toggle-group-control emotion-6 emotion-7"
class="components-toggle-group-control emotion-6 emotion-7"
data-wp-c16t="true"
data-wp-component="ToggleGroupControl"
id="toggle-group-control-0"
Expand Down
4 changes: 0 additions & 4 deletions packages/components/src/toggle-group-control/toggle-group-control-option-base/styles.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,3 @@ export const ButtonContentView = styled.div`
export const separatorActive = css`
background: transparent;
`;

export const medium = css`
min-height: ${ CONFIG.controlHeight };
`;
Comment on lines -73 to -75
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 doesn't look like it's being used anywhere.

6 changes: 3 additions & 3 deletions packages/components/src/toggle-group-control/toggle-group-control/component.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,7 @@ function ToggleGroupControl(
hideLabelFromVision = false,
help,
onChange = noop,
size = 'default',
value,
children,
...otherProps
Expand Down Expand Up @@ -83,12 +84,11 @@ function ToggleGroupControl(
const classes = useMemo(
() =>
cx(
styles.ToggleGroupControl,
styles.ToggleGroupControl( { size } ),
isBlock && styles.block,
Comment on lines +87 to 88
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 kind of wanted to refactor this so these styles go in the styles.ts file, but there were some complications. Better done in a separate PR.

'medium',
Comment on lines +87 to -88
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 was adding a .medium class on the element. Not sure what that was for 🤔 There doesn't seem to be anything in our repo that uses it, at least. I'm inclined to remove it, especially when we're adding more size variants.

Copy link
Contributor

Choose a reason for hiding this comment

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

It would be interesting to see if using git-blame we can understand if there was a specific reason for that hardcoded class to exist (just in case the removal causes any visual regression somewhere else in the codebase)

Copy link
Member Author

Choose a reason for hiding this comment

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

There are no specific mentions of it in the original PR, but looking at the component in G2 we can surmise that the medium stuff is a copy-paste artifact from the G2 sizing system.

className
),
[ className, cx, isBlock ]
[ className, cx, isBlock, size ]
);
return (
<BaseControl help={ help }>
Expand Down
24 changes: 22 additions & 2 deletions packages/components/src/toggle-group-control/toggle-group-control/styles.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,19 +8,26 @@ import styled from '@emotion/styled';
* Internal dependencies
*/
import { CONFIG, COLORS, reduceMotion } from '../../utils';
import type { ToggleGroupControlProps } from '../types';

export const ToggleGroupControl = css`
export const ToggleGroupControl = ( {
size,
}: {
size: NonNullable< ToggleGroupControlProps[ 'size' ] >;
} ) => css`
background: ${ COLORS.ui.background };
border: 1px solid;
border-color: ${ COLORS.ui.border };
border-radius: ${ CONFIG.controlBorderRadius };
display: inline-flex;
min-height: ${ CONFIG.controlHeight };
Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like the CONFIG holds a few values for controls height:

gutenberg/packages/components/src/utils/config-values.js

Lines 23 to 27 in e4252e4

controlHeight: CONTROL_HEIGHT,
controlHeightXSmall: `calc( ${ CONTROL_HEIGHT } * 0.6 )`,
controlHeightSmall: `calc( ${ CONTROL_HEIGHT } * 0.8 )`,
controlHeightLarge: `calc( ${ CONTROL_HEIGHT } * 1.2 )`,
controlHeightXLarge: `calc( ${ CONTROL_HEIGHT } * 1.4 )`,

As we work on standardizing the size variants, we should either:

  • update these values and reference them as shared config across our components
  • delete them from the shared config to avoid a scenario where some components have their height defined in "locally", and some other components reference a shared config that is out of sync

What do you think?

Copy link
Member Author

Choose a reason for hiding this comment

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

delete them from the shared config

Yes 🙈 I think this level of Logic & Order™ is hard to sustain unless they are accounted for at the design stage. Based on our current priorities, we can keep it simple for now.

Copy link
Contributor

Choose a reason for hiding this comment

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

Even though I believe that updating the config and using across components would have have been a more clear way to enforce a common height across controls, I'm also ok with deleting the common config and using "local" styles in control components for their heights.

I had a quick look and there aren't too many usages left of controlHeight, so that deletion shouldn't be too hard:

Screenshot 2022-07-13 at 18 05 50

min-width: 0;
padding: 2px;
position: relative;
transition: transform ${ CONFIG.transitionDurationFastest } linear;
${ reduceMotion( 'transition' ) }

${ toggleGroupControlSize( size ) }

&:hover {
border-color: ${ COLORS.ui.borderHover };
}
Expand All @@ -33,6 +40,19 @@ export const ToggleGroupControl = css`
}
`;

export const toggleGroupControlSize = (
size: NonNullable< ToggleGroupControlProps[ 'size' ] >
) => {
const heights = {
default: '36px',
'__unstable-large': '40px',
};

return css`
min-height: ${ heights[ size ] };
`;
};

export const block = css`
display: flex;
width: 100%;
Expand Down
6 changes: 6 additions & 0 deletions packages/components/src/toggle-group-control/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -104,6 +104,12 @@ export type ToggleGroupControlProps = Omit<
* using help property as the content.
*/
help?: ReactNode;
/**
* The size variant of the control.
*
* @default 'default'
*/
size?: 'default' | '__unstable-large';
};

export type ToggleGroupControlContextProps = RadioStateReturn & {
Expand Down