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

Fix: use checkValidity() to perform the validation in RangeControl #14322

Merged
merged 3 commits into from
Mar 16, 2019
Merged

Fix: use checkValidity() to perform the validation in RangeControl #14322

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
c6fe745
Fix: use checkValidity() to perform the validation in RangeControl
jorgefilipecosta Mar 7, 2019
0aef134
Make required opt-in; Add required to existing RangeControl usages.
jorgefilipecosta Mar 15, 2019
9b102c0
Revisions
jorgefilipecosta Mar 16, 2019
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
Next Next commit
Fix: use checkValidity() to perform the validation in RangeControl
  • Loading branch information
@jorgefilipecosta
jorgefilipecosta committed Mar 16, 2019
commit c6fe7459c10508ebf35d92fa5a84b97a24a25a11
1 change: 1 addition & 0 deletions packages/components/CHANGELOG.md
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ function RangeControl( {
min,
max,
setState,
required = true,
Copy link
Member

Choose a reason for hiding this comment

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

Can you explain how this is related to the changes, and why we're suddenly setting required as the default?

Copy link
Member Author

Choose a reason for hiding this comment

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

Hi @aduth, required ensures that an empty value is treated as false by checkValidity. We are not changing the behavior, previously by default empty values were already treated as invalid:

const newValue = event.target.value;
const newNumericValue = parseInt( newValue, 10 );
isNaN( newNumericValue ) === true

Copy link
Member

Choose a reason for hiding this comment

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

required ensures that an empty value is treated as false by checkValidity. We are not changing the behavior, previously by default empty values were already treated as invalid:

I suppose that's true, though it seems like something which ought to be opted in by the renderer. It feels a bit strange that if I rendered <RangeControl />, the element is immediately considered in a state of being invalid in whichever form its placed. I'm failing to recall why we chose to implement it the way we did, but it seems like an empty value should either be considered explicitly as an additional condition with ! checkValidity || value === '', or signalled immediately as a valid value equivalent to onChange( { value: undefined } ). I think this latter one is what we chose to avoid previously because it made some blocks need to be aware to reset their default attributes?

Copy link
Member Author

Choose a reason for hiding this comment

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

I suppose that's true, though it seems like something which ought to be opted in by the renderer. It feels a bit strange that if I rendered , the element is immediately considered in a state of being invalid in whichever form its placed.

Hi @aduth, the situation is equivalent to any case where we have a required input field.
The following field is also invalid by default until the user types something.
e.g:

<input type="text" name="usrname" required>

It seems invalid fields at the start before the user types something are a normal and expected situation.
Given the way the input field is used inside the RangeControl it also seems fine that is invalid by default. We give the option to the user to set required to false and make the component valid. But in all cases where we use RangeControl, we want a value so if we don't pass one we are in an invalid state. In most cases, the value comes from an attribute that has a default value so we will not have the range control in an invalid state.

! checkValidity || value === ''

We can follow this approach, do you think it would make sense to still have a flag that allows undefined/empty values?

or signalled immediately as a valid value equivalent to onChange( { value: undefined } ). I think this latter one is what we chose to avoid previously because it made some blocks need to be aware to reset their default attributes?

Exactly, most of the times the value comes from an attribute with a default value, and setting the attribute to undefined would not reset to the default value making the blocks not work as expected. But if we set the default values the result would also not be great. Imagine we value a RangeControl with current value 1, the attribute has a default value of 2, and the user wants to set the value to 3. If we called onChange( { value: undefined } ) as soon as the form was empty, and the default attribute was set when the attribute was undefined, when the user deleted the 1 to type 3, a value of 2 would immediately appear.

Copy link
Member

Choose a reason for hiding this comment

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

The following field is also invalid by default until the user types something.
e.g:

<input type="text" name="usrname" required>

It seems invalid fields at the start before the user types something are a normal and expected situation.

! checkValidity || value === ''

We can follow this approach, do you think it would make sense to still have a flag that allows >undefined/empty values?

To me, the main thing is that required be opt-in. In your first example, you had to literally write required for that behavior to take effect. What we're talking about in the current state of this pull request is that behavior taking effect by the default rendering of a <RangeControl />.

To your second point, I guess it depends if we can make it work where the control would signal undefined when the field is empty, and in order to get around the previous issues we had with block implementations, those usages should be updated to use required (in which case, undefined would not be passed as the value).

Would that work? Is it sensible?

Copy link
Member Author

Choose a reason for hiding this comment

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

Hi @aduth I updated the code to follow your suggestion, required is now opt-in and in our own usages, we use this option.

...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
View file
Open in desktop
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