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(number-input): adjust invalid logic and add proptype check for value #5217

Merged
merged 11 commits into from
Feb 4, 2020
Merged

Conversation

abbeyhrt
Copy link
Contributor

Closes #4294
Closes #4997
Closes #4996

Done during mob programming with @joshblack.
This PR adjusts the way that data-invalid and aria-invalid were being set by assigning all invalidation logic to isInputInvalid and adjusts the min/max valid check so that allowEmpty works as expected. This expands the proptype check for value and defaultValue to include string so that the allowEmpty state of '' doesn't create a warning and then adds a check in getDerivedStateFromProps so that a user can't enter any string except an empty one.

Changelog

New

  • test for updating value to empty string when allowEmpty is true
  • isInputInvalid logic

Changed

  • proptype for value and defaultValue

Removed

  • {{removed thing}}

Testing / Reviewing

Verify that the Number Input works as expected and that it resolves the issues from #4294, #4997, #4996. Mainly check that if allowEmpty is true, the error doesn't show up for the empty state and verify that aria-invalid and data-invalid correspond to when a number is invalid.

@abbeyhrt abbeyhrt requested a review from a team as a code owner January 30, 2020 18:16
@ghost ghost requested review from aledavila and joshblack January 30, 2020 18:17
@netlify
Copy link

netlify bot commented Jan 30, 2020

Deploy preview for the-carbon-components ready!

Built with commit bddf497

https://deploy-preview-5217--the-carbon-components.netlify.com

@netlify
Copy link

netlify bot commented Jan 30, 2020

Deploy preview for carbon-elements ready!

Built with commit 632a0c2

https://deploy-preview-5217--carbon-elements.netlify.com

@netlify
Copy link

netlify bot commented Jan 30, 2020

Deploy preview for carbon-components-react failed.

Built with commit 632a0c2

https://app.netlify.com/sites/carbon-components-react/deploys/5e3992046ac43f00072f542f

Copy link
Member

@tw15egan tw15egan left a comment

Choose a reason for hiding this comment

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

A triple whammy, nice work! Tested a few scenarios and they all work as expected 👍 ✅

@asudoh
Copy link
Contributor

asudoh commented Jan 30, 2020

@abbeyhrt Thank you for jumping in! Let me take some time to review this given it seems change in some key logic in the component. Don't hesitate to ping me if you don't hear back in a reasonable amount of time.

Copy link
Contributor

@asudoh asudoh left a comment

Choose a reason for hiding this comment

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

Put a few comments, please don't hesitate to ping me if you have any questions. Thank you again for jumping in @abbeyhrt!

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

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?

packages/react/src/components/NumberInput/NumberInput.js Outdated Show resolved Hide resolved
Copy link
Contributor

@asudoh asudoh left a comment

Choose a reason for hiding this comment

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

Thanks @abbeyhrt for all the updates!

@abbeyhrt abbeyhrt merged commit 115de18 into carbon-design-system:master Feb 4, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants