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

Jump to bottom

fix(number-input): adjust invalid logic and add proptype check for value #5217

Merged
merged 11 commits into from
Feb 4, 2020
Merged

fix(number-input): adjust invalid logic and add proptype check for value #5217

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
c375523
fix(number-input): adjust invalid logic and add proptype check for value
abbeyhrt Jan 30, 2020
2312b6e
fix(number-input): remove only from tests
abbeyhrt Jan 30, 2020
9967158
Merge branch 'master' into fix-4294
tw15egan Jan 30, 2020
59a12bb
Merge branch 'master' into fix-4294
tw15egan Jan 30, 2020
7278f6f
Merge branch 'master' into fix-4294
abbeyhrt Jan 31, 2020
c4e0786
Update packages/react/src/components/NumberInput/NumberInput.js
abbeyhrt Jan 31, 2020
bddf497
Merge branch 'master' into fix-4294
joshblack Feb 3, 2020
07072b8
fix(number-input): check for controlled state feature flag
abbeyhrt Feb 3, 2020
eb63534
Merge branch 'master' into fix-4294
abbeyhrt Feb 3, 2020
ad78667
Merge branch 'master' into fix-4294
abbeyhrt Feb 4, 2020
632a0c2
Merge branch 'master' into fix-4294
abbeyhrt Feb 4, 2020
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
15 changes: 15 additions & 0 deletions packages/react/src/components/NumberInput/NumberInput-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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('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
Expand Down
85 changes: 53 additions & 32 deletions packages/react/src/components/NumberInput/NumberInput.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
*/
Expand Down Expand Up @@ -171,14 +156,16 @@ class NumberInput extends Component {
translateWithId: id => defaultTranslations[id],
};

/**
* The DOM node refernce to the `<input>`.
* @type {HTMLInputElement}
*/
_inputRef = null;

static getDerivedStateFromProps({ min, max, value = 0 }, state) {
const { prevValue } = state;

if (typeof value === 'string' && value === '' && state.value !== '') {
Copy link
Contributor

@asudoh asudoh Jan 31, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Suggested change
if (typeof value === 'string' && value === '' && state.value !== '') {
if (!useControlledStateWithValue && value === '' && prevValue !== '') {
  • Returning non-null value, which means changing the state to the returning value, whenever value prop is an empty string and value state is not an empty string causes canceling any state update (to non-empty string, e.g. upon up/down buttons or user's type) if value prop is an empty string. The problem is observed with <NumberInput value="" /> where it doesn't allow user to make any edit at all. Wondering if you simply wanted to detect update in value prop to an empty string, which led to my suggestion above. Note that gDSFP is called for state update requests in addition to prop updates.
  • useControlledStateWithValue flag must make the entire gDSFP no-op (by returning null). The purpose of flag useControlledStateWithValue is not doing any value state update from value prop
  • typeof value === 'string' is redundant if we are doing strict equal check of value with an empty string (given strict equal check involves type checking)

Copy link
Contributor

Choose a reason for hiding this comment

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

@abbeyhrt Any thoughts on this...? Thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll make the changes for the last two points, those seem good to me 👍 Could you help me understand your first point? The intent behind it was to prevent anybody from setting the value to anything but an empty string or a number, could you provide steps for me to reproduce the problem if value is set to an empty string?

return {
value: '',
prevValue: '',
};
}

// If `useControlledStateWithValue` feature flag is on, do nothing here.
// Otherwise, do prop -> state sync with "value capping".
return useControlledStateWithValue || prevValue === value
Expand All @@ -189,6 +176,27 @@ class NumberInput extends Component {
};
}

/**
* The DOM node refernce to the `<input>`.
* @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) {
Expand Down Expand Up @@ -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) {
abbeyhrt marked this conversation as resolved.
Show resolved Hide resolved
isInputInvalid = true;
}
}
}

if (isInputInvalid) {
inputWrapperProps['data-invalid'] = true;
errorId = `${id}-error-id`;
error = (
Expand Down Expand Up @@ -391,16 +412,16 @@ class NumberInput extends Component {
{helper}
<div className={`${prefix}--number__input-wrapper`}>
<input
data-invalid={invalid || null}
aria-invalid={invalid || null}
data-invalid={isInputInvalid}
aria-invalid={isInputInvalid}
aria-describedby={errorId}
type="number"
pattern="[0-9]*"
{...other}
{...props}
ref={mergeRefs(ref, this._handleInputRef)}
/>
{invalid && (
{isInputInvalid && (
<WarningFilled16 className={`${prefix}--number__invalid`} />
)}
<div className={`${prefix}--number__controls`}>
Expand Down