Skip to content

Commit

Permalink
Fix: use checkValidity() to perform the validation in RangeControl
Browse files Browse the repository at this point in the history
  • Loading branch information
jorgefilipecosta committed Mar 14, 2019
1 parent 13334d7 commit fed092a
Show file tree
Hide file tree
Showing 4 changed files with 118 additions and 19 deletions.
1 change: 1 addition & 0 deletions packages/components/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@

- Resolves a conflict where two instance of Slot would produce an inconsistent or duplicated rendering output.
- Allow years between 0 and 1970 in DateTime component.
- Fix a bug that made `RangeControl` not work as expected with float values; Started relying on the browser validations instead of our own.

### New Feature

Expand Down
8 changes: 8 additions & 0 deletions packages/components/src/range-control/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -184,6 +184,14 @@ The maximum value accepted. If higher values are inserted onChange will not be c
- Type: `Number`
- Required: No

#### required

If true having the input field empty is invalid if false having the input field empty is valid.

- Type: `Boolean`
- Required: Yes
- Default: true

## Related components

- To collect a numerical input in a text field, use the `TextControl` component.
14 changes: 6 additions & 8 deletions packages/components/src/range-control/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ function RangeControl( {
min,
max,
setState,
required = true,
...props
} ) {
const id = `inspector-range-control-${ instanceId }`;
Expand All @@ -48,14 +49,9 @@ function RangeControl( {

const onChangeValue = ( event ) => {
const newValue = event.target.value;
const newNumericValue = parseInt( newValue, 10 );
// If the input value is invalid temporarily save it to the state,
// without calling on change.
if (
isNaN( newNumericValue ) ||
( min !== undefined && newNumericValue < min ) ||
( max !== undefined && newNumericValue > max )
) {
if ( ! event.target.checkValidity() ) {
setState( {
currentInput: newValue,
} );
Expand All @@ -64,9 +60,9 @@ function RangeControl( {
// The input is valid, reset the local state property used to temporaly save the value,
// and call onChange with the new value as a number.
resetCurrentInput();
onChange( newNumericValue );
onChange( parseFloat( newValue ) );
};
const initialSliderValue = isFinite( value ) ?
const initialSliderValue = isFinite( currentInputValue ) ?
currentInputValue :
initialPosition || '';

Expand All @@ -82,6 +78,7 @@ function RangeControl( {
className="components-range-control__slider"
id={ id }
type="range"
required={ required }
value={ initialSliderValue }
onChange={ onChangeValue }
aria-describedby={ !! help ? id + '__help' : undefined }
Expand All @@ -92,6 +89,7 @@ function RangeControl( {
<input
className="components-range-control__number"
type="number"
required={ required }
onChange={ onChangeValue }
aria-label={ label }
value={ currentInputValue }
Expand Down
114 changes: 103 additions & 11 deletions packages/components/src/range-control/test/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -42,13 +42,23 @@ describe( 'RangeControl', () => {
TestUtils.Simulate.change(
rangeInputElement(),
{
target: { value: '5' },
target: {
value: '5',
checkValidity() {
return true;
},
},
}
);
TestUtils.Simulate.change(
numberInputElement(),
{
target: { value: '10' },
target: {
value: '10',
checkValidity() {
return true;
},
},
}
);

Expand Down Expand Up @@ -96,7 +106,12 @@ describe( 'RangeControl', () => {
TestUtils.Simulate.change(
numberInputElement(),
{
target: { value: '10' },
target: {
value: '10',
checkValidity() {
return false;
},
},
}
);

Expand All @@ -116,7 +131,12 @@ describe( 'RangeControl', () => {
TestUtils.Simulate.change(
numberInputElement(),
{
target: { value: '21' },
target: {
value: '21',
checkValidity() {
return false;
},
},
}
);

Expand All @@ -136,14 +156,24 @@ describe( 'RangeControl', () => {
TestUtils.Simulate.change(
numberInputElement(),
{
target: { value: '10' },
target: {
value: '10',
checkValidity() {
return false;
},
},
}
);

TestUtils.Simulate.change(
numberInputElement(),
{
target: { value: '21' },
target: {
value: '21',
checkValidity() {
return false;
},
},
}
);

Expand All @@ -152,7 +182,12 @@ describe( 'RangeControl', () => {
TestUtils.Simulate.change(
numberInputElement(),
{
target: { value: '14' },
target: {
value: '14',
checkValidity() {
return true;
},
},
}
);

Expand All @@ -171,7 +206,12 @@ describe( 'RangeControl', () => {
TestUtils.Simulate.change(
numberInputElement(),
{
target: { value: '1' },
target: {
value: '1',
checkValidity() {
return false;
},
},
}
);

Expand All @@ -190,7 +230,12 @@ describe( 'RangeControl', () => {
TestUtils.Simulate.change(
numberInputElement(),
{
target: { value: '-101' },
target: {
value: '-101',
checkValidity() {
return false;
},
},
}
);

Expand All @@ -199,7 +244,12 @@ describe( 'RangeControl', () => {
TestUtils.Simulate.change(
numberInputElement(),
{
target: { value: '-49' },
target: {
value: '-49',
checkValidity() {
return false;
},
},
}
);

Expand All @@ -208,7 +258,49 @@ describe( 'RangeControl', () => {
TestUtils.Simulate.change(
numberInputElement(),
{
target: { value: '-50' },
target: {
value: '-50',
checkValidity() {
return true;
},
},
}
);

expect( onChange ).toHaveBeenCalled();
} );
it( 'takes into account the step starting from min', () => {
const onChange = jest.fn();
const wrapper = getWrapper( { onChange, min: 0.1, step: 0.125, value: 0.1 } );

const numberInputElement = () => TestUtils.findRenderedDOMComponentWithClass(
wrapper,
'components-range-control__number'
);

TestUtils.Simulate.change(
numberInputElement(),
{
target: {
value: '0.125',
checkValidity() {
return false;
},
},
}
);

expect( onChange ).not.toHaveBeenCalled();

TestUtils.Simulate.change(
numberInputElement(),
{
target: {
value: '0.225',
checkValidity() {
return true;
},
},
}
);

Expand Down

0 comments on commit fed092a

Please sign in to comment.