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

Fix: RangeControl does not validates the min and max properties #12952

Merged
merged 1 commit into from
Feb 18, 2019

Conversation

jorgefilipecosta
Copy link
Member

Description

This PR adds a simple min/max validation to RangeControl.
If the value is invalid it is temporarily saved in the local state and onChange is not called. During the blur event, the local state is cleared so if we had an invalid input we revert back to the last valid input. If the input is valid onChange is called as soon as the input happens.

Fixes: #12922
Related: #10791

How has this been tested?

Run the unit tests.
Verify the range controls in columns block, comments block, latest posts block and others present an expected behavior.

@jorgefilipecosta jorgefilipecosta added [Type] Bug An existing feature does not function as intended [Feature] UI Components Impacts or related to the UI component system labels Dec 17, 2018
@jorgefilipecosta jorgefilipecosta force-pushed the fix/range-control-allows-invalid-inputs branch from 867b953 to d86a5de Compare January 11, 2019 18:26
@jorgefilipecosta jorgefilipecosta force-pushed the fix/range-control-allows-invalid-inputs branch from d86a5de to 1401d72 Compare January 24, 2019 11:12
// If the input value is invalid temporarly save it to the state,
// without calling on change.
if (
( ! newNumericValue && newValue !== '0' ) ||
Copy link
Member

Choose a reason for hiding this comment

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

It took me a bit to figure out why we were doing it this way. I guess Number( '' ) coerces to 0, so we're differentiating?

Alternatively, we might consider a stricter parse, checking that the produced value is not NaN (isNaN).

For example, parseInt( '', 10 ) will produce NaN.

The main benefit I think is readability. Thoughts?

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,

It took me a bit to figure out why we were doing it this way. I guess Number( '' ) coerces to 0, so we're differentiating?

Exactly, the idea was to differentiate. I agree with your suggestion, using parseInt( '', 10 ) and isNaN improves readability and I updated the code.

onChange();
};
const resetCurrentInput = () => {
// If the input is an empty string call onChange with an undefined argument to reset to value.
Copy link
Member

Choose a reason for hiding this comment

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

Do we need to call onChange at all? If the attempted value wasn't valid, just reset the current state. The rendering parent should still think the value was the "original" since it never received any indication otherwise. Then, onChange is only ever called when the input value changes to something valid.

Copy link
Member Author

Choose a reason for hiding this comment

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

My idea was that deleting the input field would reset the field to the default value, in the same way pressing reset button does.
But that is not a requirement and just resetting to the previous value is also valid so I updated the code.

@jorgefilipecosta jorgefilipecosta force-pushed the fix/range-control-allows-invalid-inputs branch from 1401d72 to cfb0d82 Compare January 25, 2019 18:38
Copy link
Member

@aduth aduth left a comment

Choose a reason for hiding this comment

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

We should include a CHANGELOG.md entry.

if (
isNaN( newNumericValue ) ||
( min && newNumericValue < min ) ||
( max && newNumericValue > max )
Copy link
Member

Choose a reason for hiding this comment

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

Depending if we wanted to support negative numbers, the following hypothetical unit test fails:

it( 'validates when provided a max or min of zero', () => {
	const onChange = jest.fn();
	const wrapper = getWrapper( { onChange, min: -100, value: 0 } );

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

	TestUtils.Simulate.change(
		numberInputElement(),
		{
			target: { value: '1' },
		}
	);

	expect( onChange ).not.toHaveBeenCalled();
} );

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 we should support negative numbers, the logic bug was solved and this test case and one to explicitly test negative ranges were added.

@gziolo gziolo added this to the 5.1 (Gutenberg) milestone Feb 7, 2019
@gziolo
Copy link
Member

gziolo commented Feb 7, 2019

This PR needs to be updated with the latest changes from master and

We should include a CHANGELOG.md entry.

@jorgefilipecosta jorgefilipecosta force-pushed the fix/range-control-allows-invalid-inputs branch from 7eec731 to ab822e1 Compare February 11, 2019 15:50
@jorgefilipecosta
Copy link
Member Author

The changelog entry was added.

Copy link
Member

@aduth aduth left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@kadencewp
Copy link
Contributor

Hey, @jorgefilipecosta I'm not sure if this is the right place to post but this change breaks the ability to set the step of the range control to 0.1. Being able to set the step is really useful for opacity range, em sizes etc. This update breaks a number of controls in Kadence Blocks I'm sure others as well. Can I help fix this?

@kadencewp
Copy link
Contributor

@jorgefilipecosta Could just be a matter of changing line 51 to this:

let newNumericValue = parseInt( newValue, 10 );
if ( props.step !== undefined && props.step < 1 ) {
     newNumericValue = parseFloat( newValue );
}

Would it be helpful if I submitted a pull request?

@jorgefilipecosta
Copy link
Member Author

Thank you for reporting this issue @kadencethemes, I submitted a fix for it at #14322. I'm sorry for the trouble caused.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] UI Components Impacts or related to the UI component system [Type] Bug An existing feature does not function as intended
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants