FontSizePicker: add pickerMode #63040
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ciampo
commented
Jul 2, 2024
Comment on lines
-51
to
-57
| ### `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` | ||
|
|
There was a problem hiding this comment.
Moved to bottom of the list of props
ciampo
commented
Jul 2, 2024
| @@ -59,7 +59,7 @@ const FontSizePickerSelect = ( props: FontSizePickerSelectProps ) => { | |||
| __experimentalHint: hint, | |||
| }; | |||
| } ), | |||
| ...( disableCustomFontSizes ? [] : [ CUSTOM_OPTION ] ), | |||
| ...( pickerMode === 'both' ? [ CUSTOM_OPTION ] : [] ), | |||
There was a problem hiding this comment.
For now, I opted to show the "custom" option only the picker mode is both — which means that, if the mode is predefined and there is a custom value, neither the toggle group nor the select dropdown have a way to visually show that.
I thought about showing the "custom" option in the select dropdown, but then I don't really know what do if the user selects it — normally, it would switch the picker type to custom, but if the picker mode is predefined, that is not an option.
Also, in case there are less than 5 predef font sizes, the UI would show a toggle group, where there is no visual indication that the user picked a custom value. In this edge case, I thought of two solutions:
- add a "custom" toggle item (like we add a "custom" dropdown option to the select dropdown). The option could be "read-only", meaning that as soon as the user picks a predef value, the option would go away. This is the current behavior when showing the select dropdown.
- show the select dropdown even if there are fewer font size in order to enjoy the "custom" option behavior that is already there.
There was a problem hiding this comment.
For now I think it's ok to maintain the current behavior of what happens with value={ /* some custom value */ } and disableCustomFontSizes={ true }.
So, as observed on trunk, that would be:
- When predefs are a toggle group — Looks like no value is selected
- When predefs are a dropdown — "Custom" option
We'll just be clear in the docs that the consumer is responsible for any validation to prevent this state.
There was a problem hiding this comment.
That should the behavior already implemented in this PR.
One difference I could spot, though. On trunk:
- Render the default example
- Switch to custom UI and input a custom value
- Using Storybook controls, set
disableCustomFontSizestotrue - No UI renders!
In this PR:
- Render the default example
- Switch to custom UI and input a custom value
- Using Storybook controls, set
pickerModetopredefined - Un-selected toggle group control renders
I think this PR's behavior is better.
In any case, if we want to move forward with this approach, I can add more unit tests to "document" the expected behavior better.
ciampo
commented
Jul 2, 2024
| @@ -23,6 +23,7 @@ const DEFAULT_FONT_SIZE = 16; | |||
|
|
|||
| function FontSizePicker( { | |||
| fontSizes = [], | |||
| // Can this be kept as-is? | |||
There was a problem hiding this comment.
@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.
There was a problem hiding this comment.
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:
- Open a Paragraph block settings.
- Tap "Font size."
- Select a predetermined option.
- Place cursor in the custom field input.
- 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
Are you willing to replace disableCustomFontSizes with the new pickerMode prop in this file please? Happy assist with further testing as needed.
There was a problem hiding this comment.
Thank you @dcalhoun 🙏 Should we go ahead with the PR, I will apply your suggested changes and ping you again for review
ciampo
commented
Jul 2, 2024
Comment on lines
-58
to
-60
| const units = useCustomUnits( { | ||
| availableUnits: unitsProp, | ||
| } ); |
There was a problem hiding this comment.
Moved further down the file, away from picker mode / type logic
ciampo
commented
Jul 2, 2024
|
|
||
| const shouldUseSelectControl = fontSizes.length > 5; |
There was a problem hiding this comment.
Moved to a separate shouldUseSelectOverToggle util at the top of the file
ciampo
force-pushed
the
feat/font-size-picker-mode
branch
from
bdf6b94
to
cfb5c29
Compare
ciampo
commented
Jul 2, 2024
| const selectedFontSize = fontSizes.find( | ||
| ( fontSize ) => fontSize.size === value | ||
| ); | ||
| const isCustomValue = !! value && ! selectedFontSize; | ||
|
|
||
| const [ showCustomValueControl, setShowCustomValueControl ] = useState( | ||
| ! disableCustomFontSizes && isCustomValue | ||
| const [ currentPickerType, setCurrentPickerType ] = useState( |
There was a problem hiding this comment.
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 ?)
ciampo
commented
Jul 2, 2024
| ); | ||
|
|
||
| const headerHint = useMemo( () => { |
There was a problem hiding this comment.
Moved headerHint computation to an external util function to make the render function easier to read
ciampo
commented
Jul 2, 2024
Comment on lines
-153
to
-155
| { !! fontSizes.length && | ||
| shouldUseSelectControl && | ||
| ! showCustomValueControl && ( |
There was a problem hiding this comment.
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
|
Size Change: +1 B (0%) Total Size: 1.75 MB
ℹ️ View Unchanged
|
ciampo
changed the title
pickerMode, deprecated disableCustomFontSizes
ciampo
changed the title
mirka
reviewed
Jul 2, 2024
| @@ -59,7 +59,7 @@ const FontSizePickerSelect = ( props: FontSizePickerSelectProps ) => { | |||
| __experimentalHint: hint, | |||
| }; | |||
| } ), | |||
| ...( disableCustomFontSizes ? [] : [ CUSTOM_OPTION ] ), | |||
| ...( pickerMode === 'both' ? [ CUSTOM_OPTION ] : [] ), | |||
There was a problem hiding this comment.
For now I think it's ok to maintain the current behavior of what happens with value={ /* some custom value */ } and disableCustomFontSizes={ true }.
So, as observed on trunk, that would be:
- When predefs are a toggle group — Looks like no value is selected
- When predefs are a dropdown — "Custom" option
We'll just be clear in the docs that the consumer is responsible for any validation to prevent this state.
|
@mirka just wanted to check if you think that we should continue working on this PR, before I put in any further work on it |
|
@matiasbenedetto 👋 Do you have an answer to this question? Things will be more maintainable in the long run if we can figure out a coordinated approach based on your requirements. |
Hi @mirka yes, I answered in the thread: #62328 (comment) |
|
As agreed with @mirka , we will not introduce a new |
|
Closing as discussed above. Logic refactor extracted to #63553 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What?
Following this conversation, this PR introduces a new
pickerModeprop that is meant to supersede the existingdisableCustomFontSizesprop (which gets marked as deprecated).Why?
With the
disableCustomFontSizesprop, consumers of the component can choose to prevent users to enter custom font size values, forcing them to pick from the list of predefined font sizes.But there isn't a way to achieve the opposite — ie. allow users to only pick from a list of custom values, without being able to choose from a list of predef options.
The
pickerModeprop is a enum of different modes:bothmaps todisableCustomFontSizes = falsepredefinedmaps todisableCustomFontSizes = truecustomintroduces the missing mode discussed above.How?
I refactored the code and extracted some logic away from the render function, to bring more clarity to it.
Together with the
pickerModeprop, I also introduced the idea ofpickerType(see this comment).I also refactored tests and usages in Gutenberg to already use the new
pickerModeprop.TODO:
pickerMode="predefined"Testing Instructions
Note: I recommend enabling the "Hide whitespace changes" options when reviewing the code changes in this PR.
TBD
Testing Instructions for Keyboard
Screenshots or screencast