RangeControl: Integrate NumberControl + update internal state flow #23006
Conversation
Enhance RangeInput with track and rail props + style rendering. Improve inline docs for rail/utils.js
… useJumpStep hook
The reason is because we're now relying on the native HTML `input[type="number"]` incrementing/decrementing function. Jump stepping values (holding shift to jump by a multiplier, default of 10) is now being handled by the new useJumpStep hook. This hook listens to `Shift` keyboard presses and modifies the `step` prop that is passed into the `input[type="number"]` element.
…ontrol" This reverts commit 0cc791c.
ItsJonQ
added
[Package] Components
| @@ -34,20 +34,21 @@ const defaultInputProps = { | |||
| }; | |||
|
|
|||
| function useUniqueId( idProp ) { | |||
| const instanceId = useInstanceId( BoxControl ); | |||
| const id = `inspector-box-control-${ instanceId }`; | |||
| const instanceId = useInstanceId( BoxControl, 'inspector-box-control' ); | |||
There was a problem hiding this comment.
Refactored to use the prefix argument of useInstanceId
|
Size Change: +732 B (0%) Total Size: 1.13 MB
ℹ️ View Unchanged
|
| import { | ||
| inputControlActionTypes, | ||
| composeStateReducers, | ||
| } from '../input-control/state'; | ||
| import { useRTL } from '../utils/style-mixins'; | ||
| import { add, subtract, roundClamp } from '../utils/math'; |
There was a problem hiding this comment.
Abstracted various functions it new dedicated utils/*.js files, which are shared amongst various components.
| @@ -72,7 +76,7 @@ export function NumberControl( | |||
| const enableShift = event.shiftKey && isShiftStepEnabled; | |||
|
|
|||
| const incrementalValue = enableShift | |||
| ? parseFloat( shiftStep ) | |||
| ? parseFloat( shiftStep ) * parseFloat( step ) | |||
There was a problem hiding this comment.
Improvement: The modifier now accounts for shiftStep and step (used as a multiplier)
There was a problem hiding this comment.
So, if I'm understanding this correctly, this change makes it so that:
- user doesn't hold shift => values will increment by
step(ex: 5) => 5 by 5 - user holds shift => values will increment by
shiftStep(ex: 10) *step(ex: 5) => 50 by 50
The actual variation that happens when the user holds down shift is always tied to the step, and shiftStep works as an accelerator. It makes sense to me, I can't think of any cases where using two unrelated values would make sense (like step = 3 without shift and step = 7 with shift).
However, I'm wondering if this is a breaking change. It seems to me like it is, given that consumers of this API will have to change the value they pass to shiftStep to get the same results when this lands. I guess then this would require the appropriate changelog note.
There was a problem hiding this comment.
So, if I'm understanding this correctly, this change makes it so that:
@nosolosw Yes! You are correct. This became (more) evident to me when I was working with 0.01 step values. I expected the shift jump needed to be relative to the step value.
I guess then this would require the appropriate changelog note.
Happy to provide that!
There was a problem hiding this comment.
In addition to the changelog, this component's README file also needs updating to explain the new relationship between step/shiftStep.
| @@ -101,7 +105,9 @@ export function NumberControl( | |||
| if ( type === inputControlActionTypes.DRAG && isDragEnabled ) { | |||
| const { delta, shiftKey } = payload; | |||
| const [ x, y ] = delta; | |||
| const modifier = shiftKey ? shiftStep : 1; | |||
| const modifier = shiftKey | |||
There was a problem hiding this comment.
Improvement: The modifier now accounts for shiftStep and step (used as a multiplier)
| - **Tab and arrow**: The slider is controlled by using the tab key to select the thumb of the desired slider, then using arrow keys to move it. | ||
| - **Value entry field**: Discrete sliders have value entry fields. After a text entry is made, the slider position automatically updates to reflect the new value. | ||
| - **Tick marks** (Optional) Discrete sliders can use evenly spaced tick marks along the slider track, and the thumb will snap to them. Each tick mark should change the setting in increments that are discernible to the user. | ||
| - **Click and drag**: The slider is controlled by clicking the thumb and dragging it. |
There was a problem hiding this comment.
is this an editor config? I don't mind as long as we're consistent within the file (ideally, also within the repo). I'd perhaps look into why these are added? it seems weird to have 3 spaces instead of 1 in between the - and the text.
There was a problem hiding this comment.
@nosolosw I'm not sure? My VSCode auto formatted it like that. I have both Editorconfig and Prettier plugins installed.
| type={ typeProp } | ||
| value={ initialValue } | ||
| value={ valueProp } |
There was a problem hiding this comment.
I've tracked the use of this by the sub-layers and I haven't seen any change to how it's dealt with. However, before this PR the value was normalized to a floating-point number. Would it be a problem that we don't do it now?
There was a problem hiding this comment.
I just did a couple of tests.
With how the "pass-through" of props->state works now, normalizing the value is disruptive. This is applicable only when the type is text for this component. In the case of UnitControl, it uses this component underneath, but it takes care of normalizing values.
This is a little tricky to describe. Hopefully that makes sense!
In other words, it's currently structured where validation/normalization comes from the consumer, rather than the component itself. (Secretly, I'm hoping to enhance this in the future so that there's less work for consumers).
For now, I don't think there is a problem, based on currently use-cases and implementations.
There was a problem hiding this comment.
👍
So, if I'm an external (non-Gutenberg) consumer of this component, now I need to do the normalization myself, right? I guess this also needs a place in the changelog. As I mentioned in the other thread above, I'm unsure about what/how we log things in the changelog for this particular package, but IMHO, this should be logged as a breaking change.
Also: this needs to be added to the component's README.
|
Hey, tested this and it works as expected 👏 Code-wise, I'm not familiar with this part of the codebase plus it's quite a bunch to review, so I'm not comfortable approving until I understand the details and potential side-effects. I can resume my review in a few days. Hopefully, my brain has more short-term capacity by then and I'm able to navigate this quicker! |
|
Thank you so much @nosolosw !! Yes, this particular one is quite involved :). Thank you so much for taking the time to review it~! I really appreciate it |
|
Yay! Travis is happy! |
| disabled = false, | ||
| help, | ||
| initialPosition, | ||
| isShiftStepEnabled = true, |
There was a problem hiding this comment.
I've seen a few changes in props here, so we should make sure docs are in sync. At least, the isShiftEnabled prop needs to be added.
| aria-describedby={ describedBy } | ||
| aria-label={ label } | ||
| aria-hidden={ false } | ||
| className="components-range-control__slider" |
There was a problem hiding this comment.
I think the new InputRange component makes sense. However, I also feel that the components-range-control__slider class should be attached here, within the range component, and not in InputRange itself. The reason for this is that it's a class dependent on the parent component, and for homogeneity with the rest of child components.
There was a problem hiding this comment.
I can make that change. For context, the component doesn't use classNames for styling. It maintains this className based selector targeting for consumers (and tests).
There was a problem hiding this comment.
This works as expected for me and code changes make sense. I've left some minor comments in places, but they're not blocking.
The only things that we need to do are:
- number-control: update readme & mark the breaking changes in the changelog
- range-control: update readme with prop changes
- input-range: add breaking change in changelog
|
Thank you @nosolosw ! I've applied the changes and pushed it up :) |
|
Travis looks like it's happy! Will merge this up! Thanks @nosolosw ! |
(Screenshot demo'ing new track/rail color and
<NumberControl />integration)This update improves the UI/UX, performance, and flexility of
<RangeControl />. Performance has also been improved for components using the internal customuseControlledStatehook, such as<InputControl />,<NumberControl />,<UnitControl />, and<BoxControl />.🎚 RangeControl
🎨 Custom track and rail colors
<RangeControl />is enhanced to render custom track and rail colors. This concept derived from an attempt to add filters to Cover backgroundsNotice in the demo GIF (above), the rail colours are using custom gradients to better demonstrate the values that's being adjusted - not unlike what you'd might find in Adobe Photoshop.
🔢 NumberControl
The previous
input[type="number"]element has been replaced with a<NumberControl />. This unlocks some UX niceties such as Shift value jumping as well as drag to increment/decrement.⏩ Jump Stepping
Step values can now be "jumped" while pressing the arrow keys or dragging the
<RangeControl />while holding Shift. Rather than incrementing/decrementing values by step (default of1), values can now be jumped (default of10).This provides a much nicer UX, especially for making quick adjustments.
This is achieved using the new custom
useJumpStephook. The behaviours are consistent with the jump stepping features in<NumberControl />⏭
useJumpStephookThis update adds a new custom hook -
useJumpStep. This hook listens to Shift key presses on thewindow, and provides the appropriate step value used for "jumping" values.For example, if the current value is
11. By default, when Shift is held down, incrementing the value will result in20rather than12(this is the jump!)⚙️
useControlledStatehook improvementsuseControlledStatehooks receives performance and stability updates. It does this by only managing internal state if the incomingvalueis not defined (isundefinedornull). This helps better synchronize state/value updates across multiple (nested) components usinguseControlledState.useControlledStatehas also been updated to handle initial and fallback values more gracefully (rather than the consuming component managing it).Incoming/fallback values are important as different controlled values (not
undefined) can result in different behaviour in HTML elements.For example, for the
input[type="range"]in<RangeControl />, we need a fallback value ofnullto center the slider. However, otherinputtypes (e.g.numberortext) require an empty string ("").👩🔬 How has this been tested?
For testing...
npm run devUnder the "Dimensions" panel...
pxtoem) and continue adjusting height valueUnder the "Overlay" panel...
Drag the "background opacity" slider to update (press Shift for jumping)
Adjust the input field next to the slider (press Shift for jumping)
Add a Column block
Under the "Column settings" panel...
Checklist:
Related: #22278 (comment)