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.

Sign up for GitHub

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

Merged
merged 32 commits into from
Jun 26, 2020
Merged

RangeControl: Integrate NumberControl + update internal state flow #23006

Show file tree
Hide file tree
Changes from 17 commits
Commits
Show all changes
32 commits
Select commit Hold shift + click to select a range
c16c347
Improve roundFloat precision. Integrate NumberControl into RangeControl
Jun 5, 2020
6604486
Refactor forwardRef implementation for RangeControl.
Jun 5, 2020
b96c785
Improve drag interaction for NumberControl in RangeControl
Jun 5, 2020
59dbb70
Fix typo for RangeControl ForwardedComponent export
Jun 5, 2020
0ecf00d
Add jumpStep feature to range slider. Refactor value parsing and fix …
Jun 5, 2020
1697e03
Improve stability of useControlledState. Update integration of useCon…
Jun 6, 2020
7301d6f
Consoliate input jumpstep for RangeContorl and NumberControl with new…
Jun 6, 2020
68b50f5
Merge branch 'master' into update/range-control-state-flow
Jun 8, 2020
0cc791c
Remove key UP/DOWN + SHIFT tests for NumberControl and InputControl
Jun 8, 2020
2b460ae
Add tests for useJumpStep hook
Jun 8, 2020
98c62d9
Adjust JSDocs for utils/hooks and utils/math
Jun 8, 2020
cbdbf3c
Restore UP/DOWN keyboard events for NumberControl (for non type="numb…
Jun 8, 2020
b944a35
Revert "Remove key UP/DOWN + SHIFT tests for NumberControl and InputC…
Jun 8, 2020
6dbc1e9
Fix jumpStep multiplier in NumberControl
Jun 8, 2020
3b25625
Fix width responsiveness of RangeControl track vs. NumberControl
Jun 8, 2020
cad73dc
Adjust import comment spacing. Add JSDoc comments for space() util.
Jun 8, 2020
2d4e11f
Add trackColor and railColor to RangeControl README
Jun 8, 2020
417a5cd
Add JSDocs for getPrecision function
Jun 8, 2020
be1d570
Fix RangeControl selector for E2E test
Jun 8, 2020
2898b5e
Merge branch 'master' into update/range-control-state-flow
Jun 10, 2020
36ea6f1
Merge branch 'master' into update/range-control-state-flow
Jun 18, 2020
e7b7e77
Update switch logic for ns/ew cursor
Jun 18, 2020
08660ba
Simplify getDefinedValue utility
Jun 18, 2020
50ba55a
Merge branch 'master' into update/range-control-state-flow
Jun 22, 2020
ed811ea
Merge branch 'master' into update/range-control-state-flow
Jun 22, 2020
d510d27
Merge branch 'master' into update/range-control-state-flow
Jun 23, 2020
da5d94c
Update packages/components/src/utils/hooks/use-controlled-state.js
Jun 26, 2020
e907197
Merge branch 'master' into update/range-control-state-flow
Jun 26, 2020
e3ab260
Move className for InputRange to root index.js
Jun 26, 2020
5f975fb
Update shiftStep in number-control README
Jun 26, 2020
acb9759
Add isShiftStepEnabled in RangeControl README
Jun 26, 2020
c4633b7
Add note on NumberControl in CHANGELOG
Jun 26, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 6 additions & 5 deletions packages/components/src/box-control/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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' );
Copy link
Author

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


return idProp || id;
return idProp || instanceId;
}
export default function BoxControl( {
id: idProp,
inputProps = defaultInputProps,
onChange = noop,
label = __( 'Box Control' ),
values: valuesProp = DEFAULT_VALUES,
values: valuesProp,
units,
} ) {
const [ values, setValues ] = useControlledState( valuesProp );
const [ values, setValues ] = useControlledState( valuesProp, {
initial: DEFAULT_VALUES,
} );

const [ isDirty, setIsDirty ] = useState( false );
const [ isLinked, setIsLinked ] = useState( ! isValuesMixed( values ) );
Expand Down
2 changes: 1 addition & 1 deletion packages/components/src/input-control/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ import Backdrop from './backdrop';
import InputField from './input-field';
import Label from './label';
import { Container, Root, Suffix } from './styles/input-control-styles';
import { isValueEmpty } from './utils';
import { isValueEmpty } from '../utils/values';

function useUniqueId( idProp ) {
const instanceId = useInstanceId( InputControl );
Expand Down
3 changes: 2 additions & 1 deletion packages/components/src/input-control/input-field.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,9 +12,10 @@ import { UP, DOWN, ENTER } from '@wordpress/keycodes';
/**
* Internal dependencies
*/
import { useDragCursor, isValueEmpty } from './utils';
import { useDragCursor } from './utils';
import { Input } from './styles/input-control-styles';
import { useInputControlStateReducer } from './state';
import { isValueEmpty } from '../utils/values';

function InputField(
{
Expand Down
29 changes: 5 additions & 24 deletions packages/components/src/input-control/utils.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,33 +3,27 @@
*/
import { useEffect } from '@wordpress/element';

/**
* Internal dependencies
*/
import { getRTL } from '../utils/style-mixins';

/**
* Gets a CSS cursor value based on a drag direction.
*
* @param {string} dragDirection The drag direction.
* @return {string} The CSS cursor value.
*/
export function getDragCursor( dragDirection ) {
const isRtl = getRTL();
let dragCursor = 'n-resize';
let dragCursor = 'ns-resize';

switch ( dragDirection ) {
case 'n':
dragCursor = 'n-resize';
dragCursor = 'ns-resize';
ItsJonQ marked this conversation as resolved.
Show resolved Hide resolved
break;
case 'e':
dragCursor = isRtl ? 'w-resize' : 'e-resize';
dragCursor = 'ew-resize';
break;
case 's':
dragCursor = 's-resize';
dragCursor = 'ns-resize';
break;
case 'w':
dragCursor = isRtl ? 'e-resize' : 'w-resize';
dragCursor = 'ew-resize';
break;
}

Expand Down Expand Up @@ -59,16 +53,3 @@ export function useDragCursor( isDragging, dragDirection ) {

return dragCursor;
}

/**
* Determines if a value is empty, null, or undefined.
*
* @param {any} value The value to check.
* @return {boolean} Whether value is empty.
*/
export function isValueEmpty( value ) {
const isNullish = typeof value === 'undefined' || value === null;
const isEmptyString = value === '';

return isNullish || isEmptyString;
}
25 changes: 16 additions & 9 deletions packages/components/src/number-control/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
/**
* External dependencies
*/
import { clamp } from 'lodash';
import classNames from 'classnames';

/**
Expand All @@ -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';
Copy link
Author

Choose a reason for hiding this comment

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

Abstracted various functions it new dedicated utils/*.js files, which are shared amongst various components.

import { useJumpStep } from '../utils/hooks';
import { isValueEmpty } from '../utils/values';

export function NumberControl(
{
Expand All @@ -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 );
Expand All @@ -59,7 +64,6 @@ export function NumberControl(
const numberControlStateReducer = ( state, action ) => {
const { type, payload } = action;
const event = payload?.event;

const currentValue = state.value;

/**
Expand All @@ -72,7 +76,7 @@ export function NumberControl(
const enableShift = event.shiftKey && isShiftStepEnabled;

const incrementalValue = enableShift
? parseFloat( shiftStep )
? parseFloat( shiftStep ) * parseFloat( step )
Copy link
Author

Choose a reason for hiding this comment

The 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)

Copy link
Member

@oandregal oandregal Jun 18, 2020 •
edited
Loading

Choose a reason for hiding this comment

The 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:

  • 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.

Copy link
Author

Choose a reason for hiding this comment

The 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:

@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!

Copy link
Member

Choose a reason for hiding this comment

The 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
Expand Down Expand Up @@ -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
Copy link
Author

Choose a reason for hiding this comment

The 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;
Expand Down Expand Up @@ -169,8 +175,9 @@ export function NumberControl(
max={ max }
min={ min }
ref={ ref }
step={ jumpStep }
type={ typeProp }
value={ initialValue }
value={ valueProp }
Copy link
Member

Choose a reason for hiding this comment

The 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?

Copy link
Author

Choose a reason for hiding this comment

The 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 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.

Copy link
Member

@oandregal oandregal Jun 26, 2020 •
edited
Loading

Choose a reason for hiding this comment

The 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
Expand Down
Loading