Skip to content

Commit

Permalink
Remove non translatable additional info from font size picker visual …
Browse files Browse the repository at this point in the history 
…label and improve labeling (#69011)

* Remove HeaderHint from font size picker.

* Adjust tests.

* Simplify fieldset labeling to avoid repetition.

* Update custom font size range control label.

* Avoid mismatch between visual label and actual labels.

* Adjust unit tests.

* Adjust e2e test.

* Add changelog entry.

Co-authored-by: afercia <[email protected]>
Co-authored-by: carolinan <[email protected]>
  • Loading branch information
@afercia @carolinan
3 people authored Mar 3, 2025
1 parent 7c02f29 commit 2103d50
Show file tree
Hide file tree
Showing 9 changed files with 66 additions and 218 deletions.
1 change: 1 addition & 0 deletions packages/components/CHANGELOG.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@

### Bug Fixes

- `FontSizePicker`: Remove non translatable additional info from font size picker visual label and improve labeling. ([#69011](https://github.com/WordPress/gutenberg/pull/69011)).
- `Notice`: Fix text contrast for dark mode ([#69226](https://github.com/WordPress/gutenberg/pull/69226)).

## 29.4.0 (2025-02-12)
Expand Down
14 changes: 2 additions & 12 deletions packages/components/src/font-size-picker/font-size-picker-select.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7,12 +7,11 @@ import { __, sprintf } from '@wordpress/i18n';
* Internal dependencies
*/
import CustomSelectControl from '../custom-select-control';
import { parseQuantityAndUnitFromRawValue } from '../unit-control';
import type {
FontSizePickerSelectProps,
FontSizePickerSelectOption,
} from './types';
import { getCommonSizeUnit, isSimpleCssValue } from './utils';
import { isSimpleCssValue } from './utils';

const DEFAULT_OPTION: FontSizePickerSelectOption = {
key: 'default',
Expand All @@ -23,20 +22,11 @@ const DEFAULT_OPTION: FontSizePickerSelectOption = {
const FontSizePickerSelect = ( props: FontSizePickerSelectProps ) => {
const { __next40pxDefaultSize, fontSizes, value, size, onChange } = props;

const areAllSizesSameUnit = !! getCommonSizeUnit( fontSizes );

const options: FontSizePickerSelectOption[] = [
DEFAULT_OPTION,
...fontSizes.map( ( fontSize ) => {
let hint;
if ( areAllSizesSameUnit ) {
const [ quantity ] = parseQuantityAndUnitFromRawValue(
fontSize.size
);
if ( quantity !== undefined ) {
hint = String( quantity );
}
} else if ( isSimpleCssValue( fontSize.size ) ) {
if ( isSimpleCssValue( fontSize.size ) ) {
hint = String( fontSize.size );
}
return {
Expand Down
65 changes: 18 additions & 47 deletions packages/components/src/font-size-picker/index.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,8 @@ import type { ForwardedRef } from 'react';
*/
import { __ } from '@wordpress/i18n';
import { settings } from '@wordpress/icons';
import { useState, useMemo, forwardRef } from '@wordpress/element';
import { useState, forwardRef } from '@wordpress/element';
import { useInstanceId } from '@wordpress/compose';

/**
* Internal dependencies
Expand All @@ -21,20 +22,11 @@ import {
parseQuantityAndUnitFromRawValue,
useCustomUnits,
} from '../unit-control';
import { VisuallyHidden } from '../visually-hidden';
import { getCommonSizeUnit } from './utils';
import type { FontSizePickerProps } from './types';
import {
Container,
Header,
HeaderHint,
HeaderLabel,
HeaderToggle,
} from './styles';
import { Container, Header, HeaderLabel, HeaderToggle } from './styles';
import { Spacer } from '../spacer';
import FontSizePickerSelect from './font-size-picker-select';
import FontSizePickerToggleGroup from './font-size-picker-toggle-group';
import { T_SHIRT_NAMES } from './constants';
import { maybeWarnDeprecated36pxSize } from '../utils/deprecated-36px-size';

const DEFAULT_UNITS = [ 'px', 'em', 'rem', 'vw', 'vh' ];
Expand All @@ -58,6 +50,11 @@ const UnforwardedFontSizePicker = (
withReset = true,
} = props;

const labelId = useInstanceId(
UnforwardedFontSizePicker,
'font-size-picker-label'
);

const units = useCustomUnits( {
availableUnits: unitsProp,
} );
Expand All @@ -83,29 +80,6 @@ const UnforwardedFontSizePicker = (
: ( 'togglegroup' as const );
}

const headerHint = useMemo( () => {
switch ( currentPickerType ) {
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 '';
}, [ currentPickerType, selectedFontSize, fontSizes ] );

if ( fontSizes.length === 0 && disableCustomFontSizes ) {
return null;
}
Expand All @@ -131,19 +105,16 @@ const UnforwardedFontSizePicker = (
} );

return (
<Container ref={ ref } className="components-font-size-picker">
<VisuallyHidden as="legend">{ __( 'Font size' ) }</VisuallyHidden>
<Container
ref={ ref }
className="components-font-size-picker"
// This Container component renders a fieldset element that needs to be labeled.
aria-labelledby={ labelId }
>
<Spacer>
<Header className="components-font-size-picker__header">
<HeaderLabel
aria-label={ `${ __( 'Size' ) } ${ headerHint || '' }` }
>
{ __( 'Size' ) }
{ headerHint && (
<HeaderHint className="components-font-size-picker__header__hint">
{ headerHint }
</HeaderHint>
) }
<HeaderLabel id={ labelId }>
{ __( 'Font size' ) }
</HeaderLabel>
{ ! disableCustomFontSizes && (
<HeaderToggle
Expand Down Expand Up @@ -213,7 +184,7 @@ const UnforwardedFontSizePicker = (
<UnitControl
__next40pxDefaultSize={ __next40pxDefaultSize }
__shouldNotWarnDeprecated36pxSize
label={ __( 'Custom' ) }
label={ __( 'Font size' ) }
labelPosition="top"
hideLabelFromVision
value={ value }
Expand Down Expand Up @@ -245,7 +216,7 @@ const UnforwardedFontSizePicker = (
}
__shouldNotWarnDeprecated36pxSize
className="components-font-size-picker__custom-input"
label={ __( 'Custom Size' ) }
label={ __( 'Font size' ) }
hideLabelFromVision
value={ valueQuantity }
initialPosition={ fallbackFontSize }
Expand Down
5 changes: 0 additions & 5 deletions packages/components/src/font-size-picker/styles.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,6 @@ import BaseControl from '../base-control';
import Button from '../button';
import { HStack } from '../h-stack';
import { space } from '../utils/space';
import { COLORS } from '../utils';

export const Container = styled.fieldset`
border: 0;
Expand All @@ -33,7 +32,3 @@ export const HeaderLabel = styled( BaseControl.VisualLabel )`
justify-content: flex-start;
margin-bottom: 0;
`;

export const HeaderHint = styled.span`
color: ${ COLORS.gray[ 700 ] };
`;
116 changes: 36 additions & 80 deletions packages/components/src/font-size-picker/test/index.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,9 @@ describe( 'FontSizePicker', () => {
await render(
<FontSizePicker value={ value } onChange={ onChange } />
);
const input = screen.getByLabelText( 'Custom' );
const input = screen.getByRole( 'spinbutton', {
name: 'Font size',
} );
await user.clear( input );
await user.type( input, '80' );
expect( onChange ).toHaveBeenCalledTimes( 3 ); // Once for the clear, then once per keystroke.
Expand All @@ -79,7 +81,9 @@ describe( 'FontSizePicker', () => {
await user.click(
screen.getByRole( 'button', { name: 'Set custom size' } )
);
const input = screen.getByLabelText( 'Custom' );
const input = screen.getByRole( 'spinbutton', {
name: 'Font size',
} );
await user.type( input, '80' );
expect( onChange ).toHaveBeenCalledTimes( 2 ); // Once per keystroke.
expect( onChange ).toHaveBeenCalledWith( expectedValue );
Expand Down Expand Up @@ -129,36 +133,22 @@ describe( 'FontSizePicker', () => {
const options = screen.getAllByRole( 'option' );
expect( options ).toHaveLength( 7 );
expect( options[ 0 ] ).toHaveAccessibleName( 'Default' );
expect( options[ 1 ] ).toHaveAccessibleName( 'Tiny 8' );
expect( options[ 2 ] ).toHaveAccessibleName( 'Small 12' );
expect( options[ 3 ] ).toHaveAccessibleName( 'Medium 16' );
expect( options[ 4 ] ).toHaveAccessibleName( 'Large 20' );
expect( options[ 5 ] ).toHaveAccessibleName( 'Extra Large 30' );
expect( options[ 6 ] ).toHaveAccessibleName( 'xx-large 40' );
expect( options[ 1 ] ).toHaveAccessibleName( 'Tiny 8px' );
expect( options[ 2 ] ).toHaveAccessibleName( 'Small 12px' );
expect( options[ 3 ] ).toHaveAccessibleName( 'Medium 16px' );
expect( options[ 4 ] ).toHaveAccessibleName( 'Large 20px' );
expect( options[ 5 ] ).toHaveAccessibleName( 'Extra Large 30px' );
expect( options[ 6 ] ).toHaveAccessibleName( 'xx-large 40px' );
} );

test.each( [
{ value: undefined, expectedLabel: 'Size (px)' },
{ value: '8px', expectedLabel: 'Size (px)' },
{ value: '3px', expectedLabel: 'Size Custom' },
] )(
'displays $expectedLabel as label when value is $value',
async ( { value, expectedLabel } ) => {
await render(
<FontSizePicker fontSizes={ fontSizes } value={ value } />
);
expect( screen.getByLabelText( expectedLabel ) ).toBeVisible();
}
);

test.each( [
{
option: 'Default',
value: '8px',
expectedArguments: [ undefined ],
},
{
option: 'Tiny 8',
option: 'Tiny 8px',
value: undefined,
expectedArguments: [ '8px', fontSizes[ 0 ] ],
},
Expand Down Expand Up @@ -255,23 +245,6 @@ describe( 'FontSizePicker', () => {
}
);

test.each( [
{ value: undefined, expectedLabel: 'Size' },
{ value: '8px', expectedLabel: 'Size' },
{ value: '1em', expectedLabel: 'Size' },
{ value: '2rem', expectedLabel: 'Size' },
{ value: 'clamp(1.75rem, 3vw, 2.25rem)', expectedLabel: 'Size' },
{ value: '3px', expectedLabel: 'Size Custom' },
] )(
'displays $expectedLabel as label when value is $value',
async ( { value, expectedLabel } ) => {
await render(
<FontSizePicker fontSizes={ fontSizes } value={ value } />
);
expect( screen.getByLabelText( expectedLabel ) ).toBeVisible();
}
);

test.each( [
{
option: 'Default',
Expand Down Expand Up @@ -372,20 +345,6 @@ describe( 'FontSizePicker', () => {
expect( options[ 4 ] ).toHaveAccessibleName( 'Gigantosaurus' );
} );

test.each( [
{ value: undefined, expectedLabel: 'Size' },
{ value: '12px', expectedLabel: 'Size Small' },
{ value: '40px', expectedLabel: 'Size Gigantosaurus' },
] )(
'displays $expectedLabel as label when value is $value',
async ( { value, expectedLabel } ) => {
await render(
<FontSizePicker fontSizes={ fontSizes } value={ value } />
);
expect( screen.getByLabelText( expectedLabel ) ).toBeVisible();
}
);

it( 'calls onChange when a font size is selected', async () => {
const user = userEvent.setup();
const onChange = jest.fn();
Expand Down Expand Up @@ -439,25 +398,6 @@ describe( 'FontSizePicker', () => {
expect( options[ 3 ] ).toHaveAccessibleName( 'Extra Large' );
} );

test.each( [
{ value: undefined, expectedLabel: 'Size' },
{ value: '12px', expectedLabel: 'Size Small' },
{ value: '1em', expectedLabel: 'Size Medium' },
{ value: '2rem', expectedLabel: 'Size Large' },
{
value: 'clamp(1.75rem, 3vw, 2.25rem)',
expectedLabel: 'Size Extra Large',
},
] )(
'displays $expectedLabel as label when value is $value',
async ( { value, expectedLabel } ) => {
await render(
<FontSizePicker fontSizes={ fontSizes } value={ value } />
);
expect( screen.getByLabelText( expectedLabel ) ).toBeVisible();
}
);

test.each( [
{ radio: 'Small', expectedArguments: [ '12px', fontSizes[ 0 ] ] },
{ radio: 'Medium', expectedArguments: [ '1em', fontSizes[ 1 ] ] },
Expand Down Expand Up @@ -524,14 +464,18 @@ describe( 'FontSizePicker', () => {
await render(
<FontSizePicker fontSizes={ fontSizes } value="3px" />
);
expect( screen.getByLabelText( 'Custom' ) ).toBeVisible();
expect(
screen.getByRole( 'spinbutton', { name: 'Font size' } )
).toBeVisible();
} );

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

rerender(
<FontSizePicker
Expand All @@ -549,15 +493,19 @@ describe( 'FontSizePicker', () => {
const { rerender } = await render(
<FontSizePicker fontSizes={ fontSizes } value="3px" />
);
expect( screen.getByLabelText( 'Custom' ) ).toBeVisible();
expect(
screen.getByRole( 'spinbutton', { name: 'Font size' } )
).toBeVisible();

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

it( 'allows custom values by default', async () => {
Expand All @@ -569,7 +517,10 @@ describe( 'FontSizePicker', () => {
await user.click(
screen.getByRole( 'button', { name: 'Set custom size' } )
);
await user.type( screen.getByLabelText( 'Custom' ), '80' );
await user.type(
screen.getByRole( 'spinbutton', { name: 'Font size' } ),
'80'
);
expect( onChange ).toHaveBeenCalledTimes( 2 ); // Once per keystroke.
expect( onChange ).toHaveBeenCalledWith( '80px' );
} );
Expand All @@ -585,7 +536,10 @@ describe( 'FontSizePicker', () => {
screen.getByRole( 'button', { name: 'Set custom size' } )
);

await user.type( screen.getByLabelText( 'Custom' ), '80' );
await user.type(
screen.getByRole( 'spinbutton', { name: 'Font size' } ),
'80'
);

await user.click(
screen.getByRole( 'button', { name: 'Use size preset' } )
Expand Down Expand Up @@ -632,7 +586,9 @@ describe( 'FontSizePicker', () => {
await user.click(
screen.getByRole( 'button', { name: 'Set custom size' } )
);
const sliderInput = screen.getByLabelText( 'Custom Size' );
const sliderInput = screen.getByRole( 'slider', {
name: 'Font size',
} );
fireEvent.change( sliderInput, {
target: { value: 80 },
} );
Expand Down
Loading

0 comments on commit 2103d50

Please sign in to comment.