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: Use components instead of helper functions #44891

Merged
merged 30 commits into from
Nov 2, 2022
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
30 commits
Select commit Hold shift + click to select a range
dc4b183
FontSizePicker: Use components instead of helper functions
noisysocks Oct 12, 2022
bbf17a3
Remove duplicate dependency header
noisysocks Oct 12, 2022
450d63b
Reuse types from FontSizePickerProps where possible
noisysocks Oct 13, 2022
c50fa22
Rename 'number' to 'parsedValue'
noisysocks Oct 13, 2022
f882797
Use constants for defaultOption and customOption
noisysocks Oct 13, 2022
b108a6e
Don't show 'Custom' option in select when custom sizes are disabled
noisysocks Oct 13, 2022
4cdf21c
Swap label and aria-label
noisysocks Oct 13, 2022
c76c8e6
Use slug in select if name is an empty string
noisysocks Oct 13, 2022
c5e4fae
Add test that checks all the t-shirt labels
noisysocks Oct 13, 2022
6b26dc2
Merge remote-tracking branch 'origin/trunk' into update/simplify-font…
noisysocks Oct 25, 2022
97e572a
Make filenames match component names
noisysocks Oct 25, 2022
1de7d4e
Make DEFAULT_OPTION logic more explicit
noisysocks Oct 25, 2022
10bf17e
Restore headerHint logic to how it was
noisysocks Oct 25, 2022
8f87273
Use expect().toHaveContent
noisysocks Oct 25, 2022
299d680
Rewrite FontSizePicker unit tests to use userEvent and be more compre…
noisysocks Oct 26, 2022
b0314d2
Use header label's aria-label
noisysocks Oct 28, 2022
c2b4da6
Put back the Gigantosaurus 🦖
noisysocks Oct 28, 2022
d6c4d65
Update changelog
noisysocks Oct 28, 2022
9b9fc00
Merge branch 'trunk' into update/font-size-picker-unit-tests
noisysocks Oct 28, 2022
a939507
Remove editorialising comment
noisysocks Oct 31, 2022
22c9ab3
Use getByLabelText( 'Custom Size' ) to select slider
noisysocks Oct 31, 2022
8a0fc89
Remove redundant test
noisysocks Oct 31, 2022
1673ba3
Add tests for non-px font sizes
noisysocks Nov 1, 2022
ab89dde
Assert length of options/radios
noisysocks Nov 1, 2022
4032f0f
Assert number of calls to onChange
noisysocks Nov 1, 2022
41006fc
Merge branch 'update/font-size-picker-unit-tests' into update/simplif…
noisysocks Nov 1, 2022
591288b
Can un-comment this failing assertion now
noisysocks Nov 1, 2022
3af78ec
Update CHANGELOG.md
noisysocks Nov 1, 2022
bdef23a
Merge branch 'trunk' into update/simplify-font-size-picker
noisysocks Nov 1, 2022
639db98
Fix regression when value=""
noisysocks Nov 2, 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
154 changes: 48 additions & 106 deletions packages/components/src/font-size-picker/index.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ import type { ReactNode, ForwardedRef } from 'react';
* WordPress dependencies
*/
import deprecated from '@wordpress/deprecated';
import { __, sprintf } from '@wordpress/i18n';
import { __ } from '@wordpress/i18n';
import { settings } from '@wordpress/icons';
import { useState, useMemo, forwardRef } from '@wordpress/element';

Expand All @@ -18,26 +18,11 @@ import Button from '../button';
import RangeControl from '../range-control';
import { Flex, FlexItem } from '../flex';
import { default as UnitControl, useCustomUnits } from '../unit-control';
import CustomSelectControl from '../custom-select-control';
import { VisuallyHidden } from '../visually-hidden';
import {
ToggleGroupControl,
ToggleGroupControlOption,
} from '../toggle-group-control';
import {
getFontSizeOptions,
getSelectedOption,
splitValueAndUnitFromSize,
isSimpleCssValue,
CUSTOM_FONT_SIZE,
} from './utils';
import { splitValueAndUnitFromSize, isSimpleCssValue } from './utils';
import { VStack } from '../v-stack';
import { HStack } from '../h-stack';
import type {
FontSizePickerProps,
FontSizeSelectOption,
FontSizeToggleGroupOption,
} from './types';
import type { FontSizePickerProps } from './types';
import {
Container,
HeaderHint,
Expand All @@ -46,6 +31,8 @@ import {
ResetButton,
} from './styles';
import { Spacer } from '../spacer';
import FontSizePickerSelect from './select';
import FontSizePickerToggleGroup from './toggle-group';

// This conditional is needed to maintain the spacing before the slider in the `withSlider` case.
const MaybeVStack = ( {
Expand Down Expand Up @@ -77,6 +64,7 @@ const UnforwardedFontSizePicker = (
withSlider = false,
withReset = true,
} = props;

if ( ! __nextHasNoMarginBottom ) {
deprecated( 'Bottom margin styles for wp.components.FontSizePicker', {
since: '6.1',
Expand All @@ -94,28 +82,16 @@ const UnforwardedFontSizePicker = (
availableUnits: [ 'px', 'em', 'rem' ],
} );

/**
* The main font size UI displays a toggle group when the presets are less
* than six and a select control when they are more.
*/
const fontSizesContainComplexValues = fontSizes.some(
( { size: sizeArg } ) => ! isSimpleCssValue( sizeArg )
);
const shouldUseSelectControl = fontSizes.length > 5;
const options = useMemo(
() =>
getFontSizeOptions(
shouldUseSelectControl,
fontSizes,
disableCustomFontSizes
),
[ shouldUseSelectControl, fontSizes, disableCustomFontSizes ]
const selectedFontSize = fontSizes.find(
( fontSize ) => fontSize.size === value
ciampo marked this conversation as resolved.
Show resolved Hide resolved
);
const selectedOption = getSelectedOption( fontSizes, value );
const isCustomValue = selectedOption.slug === CUSTOM_FONT_SIZE;
const isCustomValue = value !== undefined && ! selectedFontSize;

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

const headerHint = useMemo( () => {
if ( showCustomValueControl ) {
return `(${ __( 'Custom' ) })`;
Expand All @@ -124,51 +100,49 @@ const UnforwardedFontSizePicker = (
// If we have a custom value that is not available in the font sizes,
// show it as a hint as long as it's a simple CSS value.
if ( isCustomValue ) {
return (
value !== undefined &&
isSimpleCssValue( value ) &&
`(${ value })`
);
return value !== undefined && isSimpleCssValue( value )
? `(${ value })`
: '';
ciampo marked this conversation as resolved.
Show resolved Hide resolved
}

if ( ! selectedFontSize ) {
return '';
}

if ( shouldUseSelectControl ) {
return (
selectedOption?.size !== undefined &&
isSimpleCssValue( selectedOption?.size ) &&
`(${ selectedOption?.size })`
);
return isSimpleCssValue( selectedFontSize.size )
? `(${ selectedFontSize.size })`
: '';
}

// Calculate the `hint` for toggle group control.
let hint = selectedOption.name;
let hint = selectedFontSize.name ?? '';
const fontSizesContainComplexValues = fontSizes.some(
( fontSize ) => ! isSimpleCssValue( fontSize.size )
);
if (
! fontSizesContainComplexValues &&
typeof selectedOption.size === 'string'
typeof selectedFontSize.size === 'string'
) {
const [ , unit ] = splitValueAndUnitFromSize( selectedOption.size );
const [ , unit ] = splitValueAndUnitFromSize(
selectedFontSize.size
);
hint += `(${ unit })`;
}
return hint;
}, [
showCustomValueControl,
selectedOption?.name,
selectedOption?.size,
value,
isCustomValue,
selectedFontSize,
value,
shouldUseSelectControl,
fontSizesContainComplexValues,
fontSizes,
] );

if ( ! options ) {
if ( fontSizes.length === 0 && disableCustomFontSizes ) {
return null;
}

// This is used for select control only. We need to add support
// for ToggleGroupControl.
const currentFontSizeSR = sprintf(
// translators: %s: Currently selected font size.
__( 'Currently selected font size: %s' ),
selectedOption.name
);
return (
<Container ref={ ref } className="components-font-size-picker">
<VisuallyHidden as="legend">{ __( 'Font size' ) }</VisuallyHidden>
Expand Down Expand Up @@ -209,64 +183,32 @@ const UnforwardedFontSizePicker = (
{ !! fontSizes.length &&
shouldUseSelectControl &&
! showCustomValueControl && (
<CustomSelectControl
__nextUnconstrainedWidth
className="components-font-size-picker__select"
label={ __( 'Font size' ) }
hideLabelFromVision
describedBy={ currentFontSizeSR }
options={ options as FontSizeSelectOption[] }
value={ (
options as FontSizeSelectOption[]
).find(
( option ) =>
option.key === selectedOption.slug
) }
onChange={ ( {
selectedItem,
}: {
selectedItem: FontSizeSelectOption;
} ) => {
<FontSizePickerSelect
fontSizes={ fontSizes }
value={ value }
size={ size }
onChange={ ( newValue ) => {
onChange?.(
hasUnits
? selectedItem.size
: Number( selectedItem.size )
hasUnits ? newValue : Number( newValue )
);
if (
selectedItem.key === CUSTOM_FONT_SIZE
) {
setShowCustomValueControl( true );
}
} }
size={ size }
onSelectCustom={ () =>
setShowCustomValueControl( true )
noisysocks marked this conversation as resolved.
Show resolved Hide resolved
}
/>
) }
{ ! shouldUseSelectControl && ! showCustomValueControl && (
<ToggleGroupControl
__nextHasNoMarginBottom={ __nextHasNoMarginBottom }
label={ __( 'Font size' ) }
hideLabelFromVision
<FontSizePickerToggleGroup
fontSizes={ fontSizes }
value={ value }
__nextHasNoMarginBottom={ __nextHasNoMarginBottom }
size={ size }
onChange={ ( newValue ) => {
onChange?.(
hasUnits ? newValue : Number( newValue )
);
} }
isBlock
size={ size }
>
{ ( options as FontSizeToggleGroupOption[] ).map(
( option ) => (
<ToggleGroupControlOption
key={ option.key }
value={ option.value }
label={ option.label }
aria-label={ option.name }
showTooltip={ true }
/>
)
) }
</ToggleGroupControl>
/>
) }
{ ! withSlider &&
! disableCustomFontSizes &&
Expand Down
77 changes: 77 additions & 0 deletions packages/components/src/font-size-picker/select.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,77 @@
/**
* WordPress dependencies
*/
import { __, sprintf } from '@wordpress/i18n';
/**
* Internal dependencies
*/
import CustomSelectControl from '../custom-select-control';

/**
* Internal dependencies
*/
import type {
FontSizePickerSelectProps,
FontSizePickerSelectOption,
} from './types';
import { splitValueAndUnitFromSize } from './utils';

const FontSizePickerSelect = ( props: FontSizePickerSelectProps ) => {
const { fontSizes = [], value, size, onChange, onSelectCustom } = props;

const defaultOption: FontSizePickerSelectOption = {
key: 'default',
name: __( 'Default' ),
value: undefined,
};
const customOption: FontSizePickerSelectOption = {
key: 'custom',
name: __( 'Custom' ),
};
noisysocks marked this conversation as resolved.
Show resolved Hide resolved
const options: FontSizePickerSelectOption[] = [
defaultOption,
...fontSizes.map( ( fontSize ) => {
const [ number ] = splitValueAndUnitFromSize( fontSize.size );
return {
key: fontSize.slug,
name: fontSize.name ?? fontSize.slug,
value: fontSize.size,
__experimentalHint: number,
noisysocks marked this conversation as resolved.
Show resolved Hide resolved
};
} ),
customOption,
noisysocks marked this conversation as resolved.
Show resolved Hide resolved
];

const selectedOption =
options.find( ( option ) => option.value === value ) ?? customOption;
Copy link
Contributor

Choose a reason for hiding this comment

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

Previously, the selectedOption was computed by matching key and slug:

option.key === selectedOption.slug

By matching the option's value instead, there's a risk of matching the wrong options, in cese two different options share the same value. I believe we should revert to using slugs instead

Copy link
Member Author

@noisysocks noisysocks Oct 13, 2022

Choose a reason for hiding this comment

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

Yes it's true that we grabbed the option with option.key === selectedOption.slug to pass to value:

value={ (
options as FontSizeSelectOption[]
).find(
( option ) =>
option.key === selectedOption.slug
) }

But drill down into where selectedOption came from and you'll see that it's the font size where fontSize.size === value:

fontSizes.find( ( font ) => font.size === value ) ||

This layer of indirection is now gone.

Copy link
Contributor

Choose a reason for hiding this comment

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

I see, thank you for the explanation!

I still wonder if this value-base check is too brittle and we're going to end up in a situation like the ColorPalette one — but this definitely seems out of scope for this PR.


return (
<CustomSelectControl
__nextUnconstrainedWidth
className="components-font-size-picker__select"
label={ __( 'Font size' ) }
hideLabelFromVision
describedBy={ sprintf(
// translators: %s: Currently selected font size.
__( 'Currently selected font size: %s' ),
selectedOption.name
) }
options={ options }
value={ selectedOption }
onChange={ ( {
selectedItem,
}: {
selectedItem: FontSizePickerSelectOption;
} ) => {
if ( selectedItem === customOption ) {
onSelectCustom();
} else {
onChange( selectedItem.value );
}
} }
size={ size }
/>
);
};

export default FontSizePickerSelect;
Loading