-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
RangeControl: Integrate NumberControl + update internal state flow #23006
Changes from all commits
c16c347
6604486
b96c785
59dbb70
0ecf00d
1697e03
7301d6f
68b50f5
0cc791c
2b460ae
98c62d9
cbdbf3c
b944a35
6dbc1e9
3b25625
cad73dc
2d4e11f
417a5cd
be1d570
2898b5e
36ea6f1
e7b7e77
08660ba
50ba55a
ed811ea
d510d27
da5d94c
e907197
e3ab260
5f975fb
acb9759
c4633b7
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,7 +1,6 @@ | ||
/** | ||
* External dependencies | ||
*/ | ||
import { clamp } from 'lodash'; | ||
import classNames from 'classnames'; | ||
|
||
/** | ||
|
@@ -13,13 +12,14 @@ import { forwardRef } from '@wordpress/element'; | |
* Internal dependencies | ||
*/ | ||
import { Input } from './styles/number-control-styles'; | ||
import { add, getValue, roundClamp, subtract } from './utils'; | ||
import { isValueEmpty } from '../input-control/utils'; | ||
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Abstracted various functions it new dedicated |
||
import { useJumpStep } from '../utils/hooks'; | ||
import { isValueEmpty } from '../utils/values'; | ||
|
||
export function NumberControl( | ||
{ | ||
|
@@ -40,9 +40,14 @@ export function NumberControl( | |
}, | ||
ref | ||
) { | ||
const initialValue = getValue( valueProp, min, max ); | ||
const baseValue = clamp( 0, min, max ); | ||
const isRtl = useRTL(); | ||
const baseValue = roundClamp( 0, min, max, step ); | ||
|
||
const jumpStep = useJumpStep( { | ||
step, | ||
shiftStep, | ||
isShiftStepEnabled, | ||
} ); | ||
|
||
const autoComplete = typeProp === 'number' ? 'off' : null; | ||
const classes = classNames( 'components-number-control', className ); | ||
|
@@ -59,7 +64,6 @@ export function NumberControl( | |
const numberControlStateReducer = ( state, action ) => { | ||
const { type, payload } = action; | ||
const event = payload?.event; | ||
|
||
const currentValue = state.value; | ||
|
||
/** | ||
|
@@ -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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Improvement: The modifier now accounts for shiftStep and step (used as a multiplier) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So, if I'm understanding this correctly, this change makes it so that:
The actual variation that happens when the user holds down shift is always tied to the 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
@nosolosw Yes! You are correct. This became (more) evident to me when I was working with
Happy to provide that! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In addition to the changelog, this component's README file also needs updating to explain the new relationship between step/shiftStep. |
||
: parseFloat( step ); | ||
let nextValue = isValueEmpty( currentValue ) | ||
? baseValue | ||
|
@@ -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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Improvement: The modifier now accounts for shiftStep and step (used as a multiplier) |
||
? parseFloat( shiftStep ) * parseFloat( step ) | ||
: parseFloat( step ); | ||
|
||
let directionModifier; | ||
let directionBaseValue; | ||
|
@@ -169,8 +175,9 @@ export function NumberControl( | |
max={ max } | ||
min={ min } | ||
ref={ ref } | ||
step={ jumpStep } | ||
type={ typeProp } | ||
value={ initialValue } | ||
value={ valueProp } | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 👍 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. |
||
__unstableStateReducer={ composeStateReducers( | ||
numberControlStateReducer, | ||
stateReducer | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Refactored to use the prefix argument of
useInstanceId