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?

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.

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.

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.

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();
} );

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.