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: add pickerMode #63040

Closed
wants to merge 12 commits into from
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@

FontSizePicker is a React component that renders a UI that allows users to select a font size.
The component renders a user interface that allows the user to select predefined (common) font sizes and contains an option that allows users to select custom font sizes (by choosing the value) if that functionality is enabled.
There is an equivalent component exposed under @wordpress/components. The difference between this component and the @wordpress components one is that this component does not require the `fontSizes` and `disableCustomFontSizes` properties. The editor settings are used to compute the value of these props.
There is an equivalent component exposed under @wordpress/components. The difference between this component and the @wordpress components one is that this component does not require the `fontSizes` and `pickerMode` properties. The editor settings are used to compute the value of these props.

## Usage

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ function FontSizePicker( props ) {
<BaseFontSizePicker
{ ...props }
fontSizes={ fontSizes }
disableCustomFontSizes={ ! customFontSize }
pickerMode={ customFontSize ? 'both' : 'predefined' }
/>
);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -209,7 +209,9 @@ export default function TypographyPanel( {

// Font Size
const hasFontSizeEnabled = useHasFontSizeControl( settings );
const disableCustomFontSizes = ! settings?.typography?.customFontSize;
const fontSizePickerMode = ! settings?.typography?.customFontSize
? 'predefined'
: 'both';
const mergedFontSizes = getMergedFontSizes( settings );

const fontSize = decodeValue( inheritedValue?.typography?.fontSize );
Expand Down Expand Up @@ -443,7 +445,7 @@ export default function TypographyPanel( {
value={ fontSize }
onChange={ setFontSize }
fontSizes={ mergedFontSizes }
disableCustomFontSizes={ disableCustomFontSizes }
pickerMode={ fontSizePickerMode }
withReset={ false }
withSlider
size="__unstable-large"
Expand Down
27 changes: 18 additions & 9 deletions packages/components/src/font-size-picker/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -48,13 +48,6 @@ const MyFontSizePicker = () => {

The component accepts the following props:

### `disableCustomFontSizes`: `boolean`

If `true`, it will not be possible to choose a custom fontSize. The user will be forced to pick one of the pre-defined sizes passed in fontSizes.

- Required: no
- Default: `false`

Comment on lines -51 to -57
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved to bottom of the list of props

### `fallbackFontSize`: `number`

If no value exists, this prop defines the starting position for the font size picker slider. Only relevant if `withSlider` is `true`.
Expand All @@ -80,6 +73,13 @@ If onChange is called without any parameter, it should reset the value, attendin

- Required: Yes

### `pickerMode`: `'predefined' | 'custom' | 'all`

When set to `predefined`, the user will be only able to pick a font size from the predefined list passed via the `fontSizes` prop. When set to `custom`, the user will be only able to choose a custom font size. When set to `all`, the user will be able to access all UIs through a toggle.

- Required: No
- Default: `'all'`

### `size`: `'default' | '__unstable-large'`

Size of the control.
Expand All @@ -102,14 +102,23 @@ The current font size value.

### `withReset`: `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` is `true`.
If `true`, a reset button will be displayed alongside the input field when a custom font size is active. Has no effect when `pickerMode` is `'predefined'` or `withSlider` is `true`.

- Required: no
- Default: `true`

### `withSlider`: `boolean`

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`.
If `true`, a slider will be displayed alongside the input field when a custom font size is active. Has no effect when `pickerMode` is `predefined`.

- Required: no
- Default: `false`

### `disableCustomFontSizes`: `boolean`

_Note: this prop is deprecated. Please use the `pickerMode` prop instead._

If `true`, it will not be possible to choose a custom font size. The user will be forced to pick one of the pre-defined sizes passed via the `fontSizes` prop.

- Required: no
- Default: `false`
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ const FontSizePickerSelect = ( props: FontSizePickerSelectProps ) => {
__next40pxDefaultSize,
fontSizes,
value,
disableCustomFontSizes,
pickerMode,
size,
onChange,
onSelectCustom,
Expand Down Expand Up @@ -59,7 +59,7 @@ const FontSizePickerSelect = ( props: FontSizePickerSelectProps ) => {
__experimentalHint: hint,
};
} ),
...( disableCustomFontSizes ? [] : [ CUSTOM_OPTION ] ),
...( pickerMode === 'all' ? [ CUSTOM_OPTION ] : [] ),
];

const selectedOption = value
Expand Down
1 change: 1 addition & 0 deletions packages/components/src/font-size-picker/index.native.js
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ const DEFAULT_FONT_SIZE = 16;

function FontSizePicker( {
fontSizes = [],
// Can this be kept as-is?
Copy link
Contributor Author

@ciampo ciampo Jul 2, 2024

Choose a reason for hiding this comment

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

@WordPress/native-mobile @dcalhoun this is still a WIP, but I wanted to check with you about the best thing to do here in case we move forward — we're basically considering deprecating the disableCustomFontSizes prop in favor of a pickerMode prop in the web version.

Copy link
Member

@dcalhoun dcalhoun Jul 5, 2024

Choose a reason for hiding this comment

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

Thanks for your patience while I found time to respond. 🙇🏻 I'm catching up on this project after being absent for a few months.

From reviewing the UI, current implementation, and proposed changes, it seems like we could likely apply the proposed changes to the native mobile editor as well. I do not perceive a specific blocker for doing so.

I quickly tested the proposed changes on a Samsung Galaxy S20 and encountered an unexpected custom field and an editor crash when:

  1. Open a Paragraph block settings.
  2. Tap "Font size."
  3. Select a predetermined option.
  4. Place cursor in the custom field input.
  5. Press the backspace key multiple times to clear the input, then press it once more.
Font size picker crash
 ERROR  TypeError: Cannot read property 'toString' of undefined

This error is located at:
    in RangeTextInput (at with-preferred-color-scheme/index.native.js:30)
    in WithPreferredColorScheme(RangeTextInput) (at stepper-cell/index.native.js:239)
    in RCTView (created by View)
    in View (at stepper.android.js:38)
    in Stepper (at with-preferred-color-scheme/index.native.js:30)
    in WithPreferredColorScheme(Stepper) (at stepper-cell/index.native.js:223)
    in RCTView (created by View)
    in View (at stepper-cell/index.native.js:221)
    in RCTView (created by View)
    in View (at cell.native.js:438)
    in RCTView (created by View)
    in View (at cell.native.js:365)
    in RCTView (created by View)
    in View (at ripple.native.js:63)
    in TouchableNativeFeedback (at ripple.native.js:53)
    in TouchableRipple (at with-preferred-color-scheme/index.native.js:30)
    in WithPreferredColorScheme(TouchableRipple) (at cell.native.js:343)
    in BottomSheetCell (at with-preferred-color-scheme/index.native.js:30)
    in WithPreferredColorScheme(BottomSheetCell) (at stepper-cell/index.native.js:203)
    in RCTView (created by View)
    in View (at stepper-cell/index.native.js:202)
    in RCTView (created by View)
    in View (at stepper-cell/index.native.js:176)
    in BottomSheetStepperCell (at with-preferred-color-scheme/index.native.js:30)
    in WithPreferredColorScheme(BottomSheetStepperCell) (at unit-control/index.native.js:165)
    in UnitControl (at with-preferred-color-scheme/index.native.js:30)
    in WithPreferredColorScheme(UnitControl) (at font-size-picker/index.native.js:167)
    in RCTView (created by View)
    in View (at font-size-picker/index.native.js:110)
    in RCTView (created by View)
    in View (at sub-sheet/index.native.js:40)
    in SlotComponent (at slot.tsx:108)
    in Slot (at slot-fill/index.native.js:11)
    in Slot (at slot-fill/index.native.js:18)
    in BottomSheetSubSheetSlot (at container.native.js:60)
    in RCTView (created by View)
    in View (at navigation-screen.native.js:115)
    in RCTView (created by View)
    in View (created by TouchableHighlight)
    in TouchableHighlight (created by TouchableHighlight)
    in TouchableHighlight (at navigation-screen.native.js:114)
    in RCTView (created by View)
    in View (created by ScrollView)
    in RCTScrollView (created by ScrollView)
    in ScrollView (created by ScrollView)
    in ScrollView (at navigation-screen.native.js:113)
    in BottomSheetNavigationScreen (at container.native.js:57)
    in StaticContainer
    in EnsureSingleNavigator (created by SceneView)
    in SceneView (created by CardContainer)
    in RCTView (created by View)
    in View (created by CardContainer)
    in RCTView (created by View)
    in View (created by CardContainer)
    in RCTView (created by View)
    in View
    in CardSheet (created by Card)
    in RCTView (created by View)
    in View
    in Unknown (created by PanGestureHandler)
    in PanGestureHandler (created by PanGestureHandler)
    in PanGestureHandler (created by Card)
    in RCTView (created by View)
    in View
    in Unknown (created by Card)
    in RCTView (created by View)
    in View (created by Card)
    in Card (created by CardContainer)
    in CardContainer (created by CardStack)
    in RCTView (created by View)
    in View
    in Unknown (created by InnerScreen)
    in InnerScreen (created by Screen)
    in Screen (created by MaybeScreen)
    in MaybeScreen (created by CardStack)
    in RCTView (created by View)
    in View (created by ScreenContainer)
    in ScreenContainer (created by MaybeScreenContainer)
    in MaybeScreenContainer (created by CardStack)
    in RCTView (created by View)
    in View (created by Background)
    in Background (created by CardStack)
    in CardStack (created by HeaderShownContext)
    in RCTView (created by View)
    in View (created by SafeAreaInsetsContext)
    in SafeAreaProviderCompat (created by StackView)
    in RNGestureHandlerRootView (created by GestureHandlerRootView)
    in GestureHandlerRootView (created by StackView)
    in StackView (created by StackNavigator)
    in PreventRemoveProvider (created by NavigationContent)
    in NavigationContent
    in Unknown (created by StackNavigator)
    in StackNavigator (at navigation-container.native.js:163)
    in EnsureSingleNavigator
    in BaseNavigationContainer
    in ThemeProvider
    in NavigationContainerInner (at navigation-container.native.js:162)
    in RCTView (created by View)
    in View (created by AnimatedComponent(View))
    in AnimatedComponent(View)
    in Unknown (at navigation-container.native.js:154)
    in BottomSheetNavigationContainer (at container.native.js:48)
    in RCTView (created by View)
    in View (at bottom-sheet/index.native.js:574)
    in RCTView (created by View)
    in View (at keyboard-avoiding-view.native.js:113)
    in KeyboardAvoidingView (at bottom-sheet/index.native.js:547)
    in RCTView (created by View)
    in View
    in Unknown (created by withAnimatable(View))
    in withAnimatable(View) (created by ReactNativeModal)
    in RCTView (created by View)
    in View (created by AppContainer)
    in RCTView (created by View)
    in View (created by AppContainer)
    in AppContainer (created by Modal)
    in RCTView (created by View)
    in View (created by Modal)
    in VirtualizedListContextResetter (created by Modal)
    in RCTModalHostView (created by Modal)
    in Modal (created by ReactNativeModal)
    in ReactNativeModal (at bottom-sheet/index.native.js:517)
    in BottomSheet (at with-preferred-color-scheme/index.native.js:30)
    in WithPreferredColorScheme(BottomSheet) (at container.native.js:39)
    in BottomSheetSettings (at layout/index.native.js:182)
    in RCTView (created by View)
    in View (created by KeyboardAvoidingView)
    in KeyboardAvoidingView (at layout/index.native.js:170)
    in RCTView (created by View)
    in View (at layout/index.native.js:149)
    in RCTView (created by View)
    in View (at tooltip/index.native.js:283)
    in TooltipSlot (at layout/index.native.js:148)
    in Layout (at with-preferred-color-scheme/index.native.js:30)
    in WithPreferredColorScheme(Layout) (at with-select/index.js:60)
    in Unknown (at pure/index.tsx:43)
    in WithSelect(WithPreferredColorScheme(Layout)) (at editor.native.js:151)
    in ErrorBoundary (at with-preferred-color-scheme/index.native.js:30)
    in WithPreferredColorScheme(ErrorBoundary) (at editor.native.js:150)
    in RNCSafeAreaProvider (created by SafeAreaProvider)
    in SafeAreaProvider (at provider/index.native.js:404)
    in BlockRefsProvider (at provider/index.native.js:28)
    in Unknown (at with-registry-provider.js:39)
    in WithRegistryProvider(Component) (at provider/index.js:291)
    in BlockContextProvider (at provider/index.js:290)
    in EntityProvider (at provider/index.js:285)
    in EntityProvider (at provider/index.js:284)
    in Unknown (at with-registry-provider.js:45)
    in WithRegistryProvider(Component) (at provider/index.js:360)
    in EditorProvider (at provider/index.native.js:399)
    in NativeEditorProvider (at with-dispatch/index.js:100)
    in WithDispatch(NativeEditorProvider) (at with-select/index.js:60)
    in Unknown (at pure/index.tsx:43)
    in WithSelect(WithDispatch(NativeEditorProvider)) (at editor.native.js:143)
    in SlotFillProvider (at editor.native.js:142)
    in RNGestureHandlerRootView (created by GestureHandlerRootView)
    in GestureHandlerRootView (at editor.native.js:141)
    in Editor (at with-dispatch/index.js:100)
    in WithDispatch(Editor) (at with-select/index.js:60)
    in Unknown (at pure/index.tsx:43)
    in WithSelect(WithDispatch(Editor)) (at src/index.native.js:39)
    in Gutenberg
    in RCTView (created by View)
    in View (created by AppContainer)
    in RCTView (created by View)
    in View (created by AppContainer)
    in AppContainer
    in gutenberg(RootComponent), js engine: hermes

Since the pickerMode prop has not been integrated here in this file, we essentially flipped the previous default by removing disableCustomFontSizes. We now show the custom value input in areas where it was once not displayed.

Unexpected custom option Paragraph block settings includes unexpected custom option

Are you willing to replace disableCustomFontSizes with the new pickerMode prop in this file please? Happy assist with further testing as needed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you @dcalhoun 🙏 Should we go ahead with the PR, I will apply your suggested changes and ping you again for review

disableCustomFontSizes = false,
onChange,
value: selectedValue,
Expand Down
190 changes: 119 additions & 71 deletions packages/components/src/font-size-picker/index.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ import type { ForwardedRef } from 'react';
*/
import { __ } from '@wordpress/i18n';
import { settings } from '@wordpress/icons';
import { useState, useMemo, forwardRef } from '@wordpress/element';
import { useState, forwardRef, useEffect } from '@wordpress/element';

/**
* Internal dependencies
Expand All @@ -23,7 +23,6 @@ import {
} from '../unit-control';
import { VisuallyHidden } from '../visually-hidden';
import { getCommonSizeUnit } from './utils';
import type { FontSizePickerProps } from './types';
import {
Container,
Header,
Expand All @@ -35,9 +34,62 @@ 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 type {
FontSize,
FontSizePickerProps,
FontSizePickerMode,
FontSizePickerType,
} from './types';

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

const shouldUseSelectOverToggle = ( howManyfontSizes: number ) =>
howManyfontSizes > 5;

const getPickerType = (
pickerMode: FontSizePickerMode,
isCustomValue: boolean,
fontSizes: FontSize[]
): FontSizePickerType => {
if (
pickerMode === 'custom' ||
( pickerMode !== 'predefined' && isCustomValue )
) {
return 'custom';
}

return shouldUseSelectOverToggle( fontSizes.length )
? 'select'
: 'togglegroup';
};

const getHeaderHint = (
currentPickerType: FontSizePickerType,
selectedFontSize: FontSize | undefined,
fontSizes: FontSize[]
) => {
if ( currentPickerType === 'custom' ) {
return __( 'Custom' );
}

if ( ! shouldUseSelectOverToggle( fontSizes.length ) ) {
if ( selectedFontSize ) {
return (
selectedFontSize.name ||
T_SHIRT_NAMES[ fontSizes.indexOf( selectedFontSize ) ]
);
}
return '';
}

const commonUnit = getCommonSizeUnit( fontSizes );
if ( commonUnit ) {
return `(${ commonUnit })`;
}

return '';
};

const UnforwardedFontSizePicker = (
props: FontSizePickerProps,
ref: ForwardedRef< any >
Expand All @@ -46,61 +98,52 @@ const UnforwardedFontSizePicker = (
__next40pxDefaultSize = false,
fallbackFontSize,
fontSizes = [],
disableCustomFontSizes = false,
onChange,
pickerMode = 'all',
size = 'default',
units: unitsProp = DEFAULT_UNITS,
value,
withSlider = false,
withReset = true,

// deprecated
disableCustomFontSizes,
} = props;

const units = useCustomUnits( {
availableUnits: unitsProp,
} );
Comment on lines -58 to -60
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved further down the file, away from picker mode / type logic

let computedPickerMode = pickerMode;
if ( disableCustomFontSizes !== undefined ) {
computedPickerMode = disableCustomFontSizes ? 'predefined' : 'all';
}

const shouldUseSelectControl = fontSizes.length > 5;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved to a separate shouldUseSelectOverToggle util at the top of the file

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

const [ showCustomValueControl, setShowCustomValueControl ] = useState(
! disableCustomFontSizes && isCustomValue
const [ currentPickerType, setCurrentPickerType ] = useState(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

While the pickerMode is an indication from the component consumer, the currentPickerType is the internal value to determine what UI is currently shows to the user (toggle group, select dropdown, or custom value UI ?)

getPickerType( computedPickerMode, isCustomValue, fontSizes )
);

const headerHint = useMemo( () => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved headerHint computation to an external util function to make the render function easier to read

if ( showCustomValueControl ) {
return __( 'Custom' );
}

if ( ! shouldUseSelectControl ) {
if ( selectedFontSize ) {
return (
selectedFontSize.name ||
T_SHIRT_NAMES[ fontSizes.indexOf( selectedFontSize ) ]
);
}
return '';
}

const commonUnit = getCommonSizeUnit( fontSizes );
if ( commonUnit ) {
return `(${ commonUnit })`;
}
useEffect( () => {
setCurrentPickerType(
getPickerType( computedPickerMode, isCustomValue, fontSizes )
);
}, [ computedPickerMode, isCustomValue, fontSizes ] );

return '';
}, [
showCustomValueControl,
shouldUseSelectControl,
selectedFontSize,
fontSizes,
] );
const units = useCustomUnits( {
availableUnits: unitsProp,
} );

if ( fontSizes.length === 0 && disableCustomFontSizes ) {
if ( fontSizes.length === 0 && computedPickerMode === 'predefined' ) {
return null;
}

const headerHint = getHeaderHint(
currentPickerType,
selectedFontSize,
fontSizes
);

// If neither the value or first font size is a string, then FontSizePicker
// operates in a legacy "unitless" mode where UnitControl can only be used
// to select px values and onChange() is always called with number values.
Expand Down Expand Up @@ -130,56 +173,60 @@ const UnforwardedFontSizePicker = (
</HeaderHint>
) }
</HeaderLabel>
{ ! disableCustomFontSizes && (
{ /* Show toggle button only when all picker modes are enabled */ }
{ computedPickerMode === 'all' && (
<HeaderToggle
label={
showCustomValueControl
currentPickerType === 'custom'
? __( 'Use size preset' )
: __( 'Set custom size' )
}
icon={ settings }
onClick={ () => {
setShowCustomValueControl(
! showCustomValueControl
setCurrentPickerType(
getPickerType(
currentPickerType === 'custom'
? 'predefined'
: 'custom',
isCustomValue,
fontSizes
)
);
} }
isPressed={ showCustomValueControl }
isPressed={ currentPickerType === 'custom' }
size="small"
/>
) }
</Header>
</Spacer>
<div>
{ !! fontSizes.length &&
shouldUseSelectControl &&
! showCustomValueControl && (
Comment on lines -153 to -155
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The logic for what UI should be shown to the user has been consolidated and moved in the getPickerType utility.

Also, the !! fontSizes.length checked seemed redundant, since shouldUseSelectControl already assumes that there are more than 5 font sizes

<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 }
pickerMode={ computedPickerMode }
size={ size }
onChange={ ( newValue ) => {
if ( newValue === undefined ) {
onChange?.( undefined );
} else {
onChange?.(
hasUnits ? newValue : Number( newValue ),
fontSizes.find(
( fontSize ) =>
fontSize.size === newValue
)
);
}
/>
) }
{ ! shouldUseSelectControl && ! showCustomValueControl && (
} }
onSelectCustom={ () =>
setCurrentPickerType( 'custom' )
}
/>
) }

{ currentPickerType === 'togglegroup' && (
<FontSizePickerToggleGroup
fontSizes={ fontSizes }
value={ value }
Expand All @@ -200,7 +247,8 @@ const UnforwardedFontSizePicker = (
} }
/>
) }
{ ! disableCustomFontSizes && showCustomValueControl && (

{ currentPickerType === 'custom' && (
<Flex className="components-font-size-picker__custom-size-control">
<FlexItem isBlock>
<UnitControl
Expand Down
Loading
Loading