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

FontSizePicker: tidy up internal logic #63553

Merged
merged 9 commits into from
Jul 18, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
1 change: 1 addition & 0 deletions packages/components/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
- `ToggleGroupControl`: support disabled options ([#63450](https://github.com/WordPress/gutenberg/pull/63450)).
- `CustomSelectControl`: Stabilize `__experimentalShowSelectedHint` and `options[]. __experimentalHint` props ([#63248](https://github.com/WordPress/gutenberg/pull/63248)).
- `SelectControl`: Add `"minimal"` variant ([#63265](https://github.com/WordPress/gutenberg/pull/63265)).
- `FontSizePicker`: tidy up internal logic ([#63553](https://github.com/WordPress/gutenberg/pull/63553)).

### Internal

Expand Down
132 changes: 68 additions & 64 deletions packages/components/src/font-size-picker/index.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,8 @@ import { T_SHIRT_NAMES } from './constants';

const DEFAULT_UNITS = [ 'px', 'em', 'rem', 'vw', 'vh' ];

const MAX_TOGGLE_GROUP_SIZES = 5;

const UnforwardedFontSizePicker = (
props: FontSizePickerProps,
ref: ForwardedRef< any >
Expand All @@ -59,43 +61,49 @@ const UnforwardedFontSizePicker = (
availableUnits: unitsProp,
} );

const shouldUseSelectControl = fontSizes.length > 5;
const selectedFontSize = fontSizes.find(
( fontSize ) => fontSize.size === value
);
const isCustomValue = !! value && ! selectedFontSize;

const [ showCustomValueControl, setShowCustomValueControl ] = useState(
! disableCustomFontSizes && isCustomValue
);

const headerHint = useMemo( () => {
if ( showCustomValueControl ) {
return __( 'Custom' );
}
// Initially request a custom picker if the value is not from the predef list.
const [ userRequestedCustom, setUserRequestedCustom ] =
useState( isCustomValue );

if ( ! shouldUseSelectControl ) {
if ( selectedFontSize ) {
return (
selectedFontSize.name ||
T_SHIRT_NAMES[ fontSizes.indexOf( selectedFontSize ) ]
);
}
return '';
}
let currentPickerType;
if ( ! disableCustomFontSizes && userRequestedCustom ) {
// While showing the custom value picker, switch back to predef only if
// `disableCustomFontSizes` is set to `true`.
currentPickerType = 'custom' as const;
} else {
currentPickerType =
fontSizes.length > MAX_TOGGLE_GROUP_SIZES
? ( 'select' as const )
: ( 'togglegroup' as const );
}

const commonUnit = getCommonSizeUnit( fontSizes );
if ( commonUnit ) {
return `(${ commonUnit })`;
const headerHint = useMemo( () => {
switch ( currentPickerType ) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Swapped the previous if-based logic to a switch-based logic looking at the value of currentPickerType, which should be much simpler to follow

case 'custom':
return __( 'Custom' );
case 'togglegroup':
if ( selectedFontSize ) {
return (
selectedFontSize.name ||
T_SHIRT_NAMES[ fontSizes.indexOf( selectedFontSize ) ]
);
}
break;
case 'select':
const commonUnit = getCommonSizeUnit( fontSizes );
if ( commonUnit ) {
return `(${ commonUnit })`;
}
break;
}

return '';
}, [
showCustomValueControl,
shouldUseSelectControl,
selectedFontSize,
fontSizes,
] );
}, [ currentPickerType, selectedFontSize, fontSizes ] );

if ( fontSizes.length === 0 && disableCustomFontSizes ) {
return null;
Expand Down Expand Up @@ -133,53 +141,45 @@ const UnforwardedFontSizePicker = (
{ ! disableCustomFontSizes && (
<HeaderToggle
label={
showCustomValueControl
currentPickerType === 'custom'
? __( 'Use size preset' )
: __( 'Set custom size' )
}
icon={ settings }
onClick={ () => {
setShowCustomValueControl(
! showCustomValueControl
);
} }
isPressed={ showCustomValueControl }
onClick={ () =>
setUserRequestedCustom( ! userRequestedCustom )
}
isPressed={ currentPickerType === 'custom' }
size="small"
/>
) }
</Header>
</Spacer>
<div>
{ !! fontSizes.length &&
shouldUseSelectControl &&
! showCustomValueControl && (
<FontSizePickerSelect
__next40pxDefaultSize={ __next40pxDefaultSize }
fontSizes={ fontSizes }
value={ value }
disableCustomFontSizes={ disableCustomFontSizes }
size={ size }
onChange={ ( newValue ) => {
if ( newValue === undefined ) {
onChange?.( undefined );
} else {
onChange?.(
hasUnits
? newValue
: Number( newValue ),
fontSizes.find(
( fontSize ) =>
fontSize.size === newValue
)
);
}
} }
onSelectCustom={ () =>
setShowCustomValueControl( true )
{ currentPickerType === 'select' && (
<FontSizePickerSelect
__next40pxDefaultSize={ __next40pxDefaultSize }
fontSizes={ fontSizes }
value={ value }
disableCustomFontSizes={ disableCustomFontSizes }
size={ size }
onChange={ ( newValue ) => {
if ( newValue === undefined ) {
onChange?.( undefined );
} else {
onChange?.(
hasUnits ? newValue : Number( newValue ),
fontSizes.find(
( fontSize ) =>
fontSize.size === newValue
)
);
}
/>
) }
{ ! shouldUseSelectControl && ! showCustomValueControl && (
} }
onSelectCustom={ () => setUserRequestedCustom( true ) }
/>
) }
{ currentPickerType === 'togglegroup' && (
<FontSizePickerToggleGroup
fontSizes={ fontSizes }
value={ value }
Expand All @@ -200,7 +200,7 @@ const UnforwardedFontSizePicker = (
} }
/>
) }
{ ! disableCustomFontSizes && showCustomValueControl && (
{ currentPickerType === 'custom' && (
<Flex className="components-font-size-picker__custom-size-control">
<FlexItem isBlock>
<UnitControl
Expand All @@ -210,6 +210,8 @@ const UnforwardedFontSizePicker = (
hideLabelFromVision
value={ value }
onChange={ ( newValue ) => {
setUserRequestedCustom( true );

if ( newValue === undefined ) {
onChange?.( undefined );
} else {
Expand Down Expand Up @@ -240,6 +242,8 @@ const UnforwardedFontSizePicker = (
initialPosition={ fallbackFontSize }
withInputField={ false }
onChange={ ( newValue ) => {
setUserRequestedCustom( true );

if ( newValue === undefined ) {
onChange?.( undefined );
} else if ( hasUnits ) {
Expand Down
98 changes: 83 additions & 15 deletions packages/components/src/font-size-picker/test/index.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,29 @@ import userEvent from '@testing-library/user-event';
*/
import FontSizePicker from '../';
import type { FontSize } from '../types';
/**
* WordPress dependencies
*/
import { useState } from '@wordpress/element';

const ControlledFontSizePicker = ( {
onChange,
...props
}: React.ComponentProps< typeof FontSizePicker > ) => {
const [ fontSize, setFontSize ] =
useState< typeof props.value >( undefined );

return (
<FontSizePicker
{ ...props }
value={ fontSize }
onChange={ ( newFontSize ) => {
setFontSize( newFontSize );
onChange?.( newFontSize );
} }
/>
);
};

describe( 'FontSizePicker', () => {
test.each( [
Expand Down Expand Up @@ -118,9 +141,7 @@ describe( 'FontSizePicker', () => {
render(
<FontSizePicker fontSizes={ fontSizes } value={ value } />
);
expect(
screen.getByLabelText( expectedLabel )
).toBeInTheDocument();
expect( screen.getByLabelText( expectedLabel ) ).toBeVisible();
}
);

Expand Down Expand Up @@ -243,9 +264,7 @@ describe( 'FontSizePicker', () => {
render(
<FontSizePicker fontSizes={ fontSizes } value={ value } />
);
expect(
screen.getByLabelText( expectedLabel )
).toBeInTheDocument();
expect( screen.getByLabelText( expectedLabel ) ).toBeVisible();
}
);

Expand Down Expand Up @@ -360,9 +379,7 @@ describe( 'FontSizePicker', () => {
render(
<FontSizePicker fontSizes={ fontSizes } value={ value } />
);
expect(
screen.getByLabelText( expectedLabel )
).toBeInTheDocument();
expect( screen.getByLabelText( expectedLabel ) ).toBeVisible();
}
);

Expand Down Expand Up @@ -434,9 +451,7 @@ describe( 'FontSizePicker', () => {
render(
<FontSizePicker fontSizes={ fontSizes } value={ value } />
);
expect(
screen.getByLabelText( expectedLabel )
).toBeInTheDocument();
expect( screen.getByLabelText( expectedLabel ) ).toBeVisible();
}
);

Expand Down Expand Up @@ -493,7 +508,7 @@ describe( 'FontSizePicker', () => {
render(
<FontSizePicker fontSizes={ fontSizes } value={ value } />
);
expect( screen.getByRole( 'radiogroup' ) ).toBeInTheDocument();
expect( screen.getByRole( 'radiogroup' ) ).toBeVisible();
expect(
screen.queryByRole( 'radio', { checked: true } )
).not.toBeInTheDocument();
Expand All @@ -514,15 +529,48 @@ describe( 'FontSizePicker', () => {
await user.click(
screen.getByRole( 'option', { name: 'Custom' } )
);
expect( screen.getByLabelText( 'Custom' ) ).toBeInTheDocument();
expect( screen.getByLabelText( 'Custom' ) ).toBeVisible();
expect( onChange ).not.toHaveBeenCalled();
} );
}

function commonTests( fontSizes: FontSize[] ) {
it( 'shows custom input when value is unknown', () => {
render( <FontSizePicker fontSizes={ fontSizes } value="3px" /> );
expect( screen.getByLabelText( 'Custom' ) ).toBeInTheDocument();
expect( screen.getByLabelText( 'Custom' ) ).toBeVisible();
} );

it( 'hides custom input when disableCustomFontSizes is set to `true` with a custom font size', () => {
const { rerender } = render(
<FontSizePicker fontSizes={ fontSizes } value="3px" />
);
expect( screen.getByLabelText( 'Custom' ) ).toBeVisible();

rerender(
<FontSizePicker
disableCustomFontSizes
fontSizes={ fontSizes }
value="3px"
/>
);
expect(
screen.queryByLabelText( 'Custom' )
).not.toBeInTheDocument();
Comment on lines +556 to +558
Copy link
Contributor Author

@ciampo ciampo Jul 15, 2024

Choose a reason for hiding this comment

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

I didn't write a more specific assertion on which UI is presented to the user since I initially wrote this unit test so that it would pass on trunk, where no input UI is actually presented to the user in this case (a bug IMO — see a more extended explanation in the PR description).

Since the input UI presented to the user changes depending on the length of the fontSizes prop, if we think that we should test that part too, I will duplicate this test and add it to two different sections of the file

} );

it( "doesn't hide custom input when the selected font size is a predef", () => {
const { rerender } = render(
<FontSizePicker fontSizes={ fontSizes } value="3px" />
);
expect( screen.getByLabelText( 'Custom' ) ).toBeVisible();

rerender(
<FontSizePicker
fontSizes={ fontSizes }
value={ fontSizes[ 0 ].size }
/>
);
expect( screen.getByLabelText( 'Custom' ) ).toBeVisible();
} );

it( 'allows custom values by default', async () => {
Expand All @@ -539,6 +587,26 @@ describe( 'FontSizePicker', () => {
expect( onChange ).toHaveBeenCalledWith( '80px' );
} );

it( 'allows switching between custom and predef inputs when pressing the dedicated toggle', async () => {
const user = userEvent.setup();

render( <ControlledFontSizePicker fontSizes={ fontSizes } /> );

await user.click(
screen.getByRole( 'button', { name: 'Set custom size' } )
);

await user.type( screen.getByLabelText( 'Custom' ), '80' );

await user.click(
screen.getByRole( 'button', { name: 'Use size preset' } )
);

expect(
screen.queryByLabelText( 'Custom' )
).not.toBeInTheDocument();
} );

it( 'does not allow custom values when disableCustomFontSizes is set', () => {
render(
<FontSizePicker
Expand Down
7 changes: 4 additions & 3 deletions packages/components/src/font-size-picker/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -37,16 +37,17 @@ export type FontSizePickerProps = {
*/
value?: number | string;
/**
* If `true`, the UI will contain a slider, instead of a numeric text input
* field. If `false`, no slider will be present.
* If `true`, a slider will be displayed alongside the input field when a
* custom font size is active. Has no effect when `disableCustomFontSizes`
* is `true`.
*
* @default false
*/
withSlider?: boolean;
/**
* If `true`, a reset button will be displayed alongside the input field
* when a custom font size is active. Has no effect when
* `disableCustomFontSizes` or `withSlider` is `true`.
Copy link
Member

Choose a reason for hiding this comment

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

Good catch, seems like this was wrong.

* `disableCustomFontSizes` is `true`.
*
* @default true
*/
Expand Down
Loading