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

RangeControl: Fix number input change interaction #22084

Merged
merged 11 commits into from
May 11, 2020
26 changes: 12 additions & 14 deletions packages/components/src/range-control/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -30,13 +30,13 @@ import {
AfterIconWrapper,
BeforeIconWrapper,
InputRange,
InputNumber,
Root,
Track,
ThumbWrapper,
Thumb,
Wrapper,
} from './styles/range-control-styles';
import InputField from './input-field';
import { useRtl } from '../utils/rtl';

const BaseRangeControl = forwardRef(
Expand All @@ -61,6 +61,7 @@ const BaseRangeControl = forwardRef(
onFocus = noop,
onMouseMove = noop,
onMouseLeave = noop,
resetFallbackValue,
Copy link
Member

Choose a reason for hiding this comment

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

What's the prop needed for? Could we include documentation?

Copy link
Member

Choose a reason for hiding this comment

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

What's the prop needed for? Could we include documentation?

Still wondering about this one.

Copy link
Author

Choose a reason for hiding this comment

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

Ah! Thanks for following up. It's the value to be used if the "Reset" button is clicked.

This was a prop that existed previously, but I guess didn't have docs. I'll add some.

renderTooltipContent = ( v ) => v,
showTooltip: showTooltipProp,
step = 1,
Expand Down Expand Up @@ -122,13 +123,6 @@ const BaseRangeControl = forwardRef(
const enableTooltip = showTooltipProp !== false && isFinite( value );

const handleOnChange = ( event ) => {
if (
event.target.checkValidity &&
! event.target.checkValidity()
) {
return;
}

const nextValue = parseFloat( event.target.value );

if ( isNaN( nextValue ) ) {
Expand All @@ -141,7 +135,13 @@ const BaseRangeControl = forwardRef(
};

const handleOnReset = () => {
setValue( null );
let resetValue = parseFloat( resetFallbackValue );

if ( isNaN( resetValue ) ) {
resetValue = null;
}

setValue( resetValue );
onChange( undefined );
};

Expand Down Expand Up @@ -250,16 +250,14 @@ const BaseRangeControl = forwardRef(
</AfterIconWrapper>
) }
{ withInputField && (
<InputNumber
aria-label={ label }
className="components-range-control__number"
<InputField
disabled={ disabled }
inputMode="decimal"
label={ label }
max={ max }
min={ min }
onChange={ handleOnChange }
onReset={ handleOnReset }
step={ step }
type="number"
value={ inputSliderValue }
/>
) }
Expand Down
105 changes: 105 additions & 0 deletions packages/components/src/range-control/input-field.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,105 @@
/**
* External dependencies
*/

import { noop } from 'lodash';
ItsJonQ marked this conversation as resolved.
Show resolved Hide resolved
/**
* WordPress dependencies
*/
import { ENTER } from '@wordpress/keycodes';

/**
* Internal dependencies
*/
import { useControlledState } from '../utils/hooks';
import { InputNumber } from './styles/range-control-styles';

export default function InputField( {
label,
onBlur = noop,
onChange = noop,
onReset = noop,
onKeyDown = noop,
value: valueProp,
...props
} ) {
/**
* This component stores an internal (input) value state, derived from
* the incoming value prop.
*
* This allows for the <input /> to be updated independently before the
* value is applied and propagated. This independent updating action is
* necessary to accommodate individual keystroke values that may not
* be considered "valid" (e.g. within the min - max range).
*/
const [ value, setValue ] = useControlledState( valueProp );

const handleOnReset = ( event ) => {
onReset( event );
setValue( '' );
};

const handleOnSubmit = ( event ) => {
const nextValue = parseFloat( event.target.value );

if ( isNaN( nextValue ) ) {
handleOnReset();
return;
}

// Only propagate the event if the value is valid.
if ( event.target.checkValidity && ! event.target.checkValidity() ) {
// Otherwise... reset to initial value
setValue( valueProp );
return;
}
onChange( event );
};

const handleOnBlur = ( event ) => {
onBlur( event );
handleOnSubmit( event );
};

const handleOnChange = ( event ) => {
setValue( event.target.value );

/**
* Prevent submitting if changes are invalid.
* This only applies to values being entered via KEY_DOWN.
*
* Pressing the up/down arrows of the HTML input also triggers a
* change event. However, those values will be (pre)validated by the
* HTML input.
*/
if ( event.target.checkValidity && ! event.target.checkValidity() ) {
return;
}

handleOnSubmit( event );
Copy link
Member

Choose a reason for hiding this comment

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

If I understand "submit" in this context to mean "propagate a value which is valid", I'd worry about the specific word "submit", since it typically carries a very different meaning when considering an input of a form (i.e. the form submission). At least, that was my first impression seeing this.

A word like "commit" or "persist" might be less prone to confusion?

Copy link
Author

Choose a reason for hiding this comment

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

Thank you @aduth ! That makes sense. I think commit works! I'll make the update here (as well as my other PR that uses a similar flow)

};

const handleOnKeyDown = ( event ) => {
const { keyCode } = event;
onKeyDown( event );

if ( keyCode === ENTER ) {
event.preventDefault();
handleOnSubmit( event );
}
};

return (
<InputNumber
aria-label={ label }
className="components-range-control__number"
inputMode="decimal"
onBlur={ handleOnBlur }
onChange={ handleOnChange }
onKeyDown={ handleOnKeyDown }
type="number"
value={ value }
{ ...props }
/>
);
}
4 changes: 2 additions & 2 deletions packages/components/src/range-control/rail.js
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,7 @@ function useMarks( { marks, min = 0, max = 100, step = 1, value = 0 } ) {

const isCustomMarks = Array.isArray( marks );

const markCount = ( max - min ) / step;
const markCount = Math.round( ( max - min ) / step );
const marksArray = isCustomMarks
? marks
: [ ...Array( markCount + 1 ) ].map( ( _, index ) => ( {
Expand All @@ -77,7 +77,7 @@ function useMarks( { marks, min = 0, max = 100, step = 1, value = 0 } ) {
const markValue = mark.value !== undefined ? mark.value : value;

const key = `mark-${ index }`;
const isFilled = markValue * step <= value;
const isFilled = markValue * step + min <= value;
const offset = `${ ( markValue / markCount ) * 100 }%`;

const offsetStyle = {
Expand Down
Loading