From c3755234ac04b960856750446d4685d91b60d20f Mon Sep 17 00:00:00 2001 From: Abbey Hart Date: Thu, 30 Jan 2020 11:53:13 -0600 Subject: [PATCH 1/4] fix(number-input): adjust invalid logic and add proptype check for value --- .../NumberInput/NumberInput-test.js | 15 ++++ .../src/components/NumberInput/NumberInput.js | 85 ++++++++++++------- 2 files changed, 68 insertions(+), 32 deletions(-) diff --git a/packages/react/src/components/NumberInput/NumberInput-test.js b/packages/react/src/components/NumberInput/NumberInput-test.js index aa6c4cc2bc3c..4b9d6366e583 100644 --- a/packages/react/src/components/NumberInput/NumberInput-test.js +++ b/packages/react/src/components/NumberInput/NumberInput-test.js @@ -198,12 +198,27 @@ describe('NumberInput', () => { .find('NumberInput') .instance() .setState({ value: '' }); + wrapper.update(); wrapper.setProps({ allowEmpty: true }); const invalidText = wrapper.find(`.${prefix}--form-requirement`); expect(invalidText.length).toEqual(0); }); + it.only('should allow updating the value with empty string and not be invalid', () => { + // Enzyme doesn't seem to allow setState() in a forwardRef-wrapped class component + wrapper + .find('NumberInput') + .instance() + .setState({ value: 50 }); + + wrapper.update(); + wrapper.setProps({ value: '', allowEmpty: true }); + wrapper.update(); + const invalidText = wrapper.find(`.${prefix}--form-requirement`); + expect(invalidText.length).toEqual(0); + }); + it('should change the value upon change in props', () => { wrapper.setProps({ value: 1 }); // Enzyme doesn't seem to allow setState() in a forwardRef-wrapped class component diff --git a/packages/react/src/components/NumberInput/NumberInput.js b/packages/react/src/components/NumberInput/NumberInput.js index e7978e2060c0..b1d8dcad7aa8 100644 --- a/packages/react/src/components/NumberInput/NumberInput.js +++ b/packages/react/src/components/NumberInput/NumberInput.js @@ -40,21 +40,6 @@ const capMax = (max, value) => : Math.min(max, value); class NumberInput extends Component { - constructor(props) { - super(props); - this.isControlled = props.value !== undefined; - if (useControlledStateWithValue && this.isControlled) { - // Skips the logic of setting initial state if this component is controlled - return; - } - let value = useControlledStateWithValue ? props.defaultValue : props.value; - value = value === undefined ? 0 : value; - if (props.min || props.min === 0) { - value = Math.max(props.min, value); - } - this.state = { value }; - } - static propTypes = { /** * Specify an optional className to be applied to the wrapper node @@ -113,11 +98,11 @@ class NumberInput extends Component { /** * Optional starting value for uncontrolled state */ - defaultValue: PropTypes.number, + defaultValue: PropTypes.oneOfType([PropTypes.number, PropTypes.string]), /** * Specify the value of the input */ - value: PropTypes.number, + value: PropTypes.oneOfType([PropTypes.number, PropTypes.string]), /** * Specify if the component should be read-only */ @@ -171,14 +156,16 @@ class NumberInput extends Component { translateWithId: id => defaultTranslations[id], }; - /** - * The DOM node refernce to the ``. - * @type {HTMLInputElement} - */ - _inputRef = null; - static getDerivedStateFromProps({ min, max, value = 0 }, state) { const { prevValue } = state; + + if (typeof value === 'string' && value === '' && state.value !== '') { + return { + value: '', + prevValue: '', + }; + } + // If `useControlledStateWithValue` feature flag is on, do nothing here. // Otherwise, do prop -> state sync with "value capping". return useControlledStateWithValue || prevValue === value @@ -189,6 +176,27 @@ class NumberInput extends Component { }; } + /** + * The DOM node refernce to the ``. + * @type {HTMLInputElement} + */ + _inputRef = null; + + constructor(props) { + super(props); + this.isControlled = props.value !== undefined; + if (useControlledStateWithValue && this.isControlled) { + // Skips the logic of setting initial state if this component is controlled + return; + } + let value = useControlledStateWithValue ? props.defaultValue : props.value; + value = value === undefined ? 0 : value; + if (props.min || props.min === 0) { + value = Math.max(props.min, value); + } + this.state = { value }; + } + handleChange = evt => { const { disabled, onChange } = this.props; if (!disabled) { @@ -310,12 +318,25 @@ class NumberInput extends Component { const inputWrapperProps = {}; let errorId = null; let error = null; - if ( - invalid || - (!allowEmpty && this.state.value === '') || - this.state.value > max || - this.state.value < min - ) { + + let isInputInvalid; + + // If the user supplied `invalid` through props, we'll defer to the passed in value + if (invalid) { + isInputInvalid = true; + } else { + // Otherwise, if we don't allow an empty value then we check to see + // if the value is empty, or if it is out of range + if (!allowEmpty && this.state.value === '') { + isInputInvalid = true; + } else { + if (this.state.value > max || this.state.value < min) { + isInputInvalid = true; + } + } + } + + if (isInputInvalid) { inputWrapperProps['data-invalid'] = true; errorId = `${id}-error-id`; error = ( @@ -391,8 +412,8 @@ class NumberInput extends Component { {helper}
- {invalid && ( + {isInputInvalid && ( )}
From 2312b6eca418d5a200d395e0e73357a2b8ecdfa5 Mon Sep 17 00:00:00 2001 From: Abbey Hart Date: Thu, 30 Jan 2020 11:55:50 -0600 Subject: [PATCH 2/4] fix(number-input): remove only from tests --- packages/react/src/components/NumberInput/NumberInput-test.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/react/src/components/NumberInput/NumberInput-test.js b/packages/react/src/components/NumberInput/NumberInput-test.js index 4b9d6366e583..b3080c10c026 100644 --- a/packages/react/src/components/NumberInput/NumberInput-test.js +++ b/packages/react/src/components/NumberInput/NumberInput-test.js @@ -205,7 +205,7 @@ describe('NumberInput', () => { expect(invalidText.length).toEqual(0); }); - it.only('should allow updating the value with empty string and not be invalid', () => { + it('should allow updating the value with empty string and not be invalid', () => { // Enzyme doesn't seem to allow setState() in a forwardRef-wrapped class component wrapper .find('NumberInput') From c4e07860394959f99883452f2dd9b5325a3b45bf Mon Sep 17 00:00:00 2001 From: Abbey Hart Date: Fri, 31 Jan 2020 10:42:38 -0600 Subject: [PATCH 3/4] Update packages/react/src/components/NumberInput/NumberInput.js Co-Authored-By: Akira Sudoh --- packages/react/src/components/NumberInput/NumberInput.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/react/src/components/NumberInput/NumberInput.js b/packages/react/src/components/NumberInput/NumberInput.js index b1d8dcad7aa8..1f8e0940e37f 100644 --- a/packages/react/src/components/NumberInput/NumberInput.js +++ b/packages/react/src/components/NumberInput/NumberInput.js @@ -330,7 +330,7 @@ class NumberInput extends Component { if (!allowEmpty && this.state.value === '') { isInputInvalid = true; } else { - if (this.state.value > max || this.state.value < min) { + if (this.state.value !== '' && (this.state.value > max || this.state.value < min)) { isInputInvalid = true; } } From 07072b8a2f267b728ec0319804f38e54963ce907 Mon Sep 17 00:00:00 2001 From: Abbey Hart Date: Mon, 3 Feb 2020 17:35:15 -0600 Subject: [PATCH 4/4] fix(number-input): check for controlled state feature flag --- packages/react/src/components/NumberInput/NumberInput.js | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/packages/react/src/components/NumberInput/NumberInput.js b/packages/react/src/components/NumberInput/NumberInput.js index 1f8e0940e37f..efffd56d5c38 100644 --- a/packages/react/src/components/NumberInput/NumberInput.js +++ b/packages/react/src/components/NumberInput/NumberInput.js @@ -159,7 +159,7 @@ class NumberInput extends Component { static getDerivedStateFromProps({ min, max, value = 0 }, state) { const { prevValue } = state; - if (typeof value === 'string' && value === '' && state.value !== '') { + if (useControlledStateWithValue && value === '' && prevValue !== '') { return { value: '', prevValue: '', @@ -330,7 +330,10 @@ class NumberInput extends Component { if (!allowEmpty && this.state.value === '') { isInputInvalid = true; } else { - if (this.state.value !== '' && (this.state.value > max || this.state.value < min)) { + if ( + this.state.value !== '' && + (this.state.value > max || this.state.value < min) + ) { isInputInvalid = true; } }