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

[EuiFieldNumber] Improve step validation #7202

Closed
wants to merge 6 commits into from
Closed

[EuiFieldNumber] Improve step validation #7202

wants to merge 6 commits into from

Conversation

cee-chen
Copy link
Member

@cee-chen cee-chen commented Sep 19, 2023 •
edited
Loading

Summary

closes #7180

As always, I recommend following along by commit as this PR contains some incidental cleanup.

QA

  • Go to https://eui.elastic.co/pr_7202/#/forms/form-controls#number-field
  • Open the Playground flyout (bottom right hand corner of the demo)
  • Set the step input to any number
  • Type a value into the playground demo that is not divisible by the step
  • Confirm that an error icon shows up, and does not go away when clicking outside the input (unlike production)
  • Hover over the error icon/input and confirm that you see a meaningful error message in the input tooltip
  • Type a new value into the demo that is divisible by the step
  • Confirm the error icon is removed
  • On a Webkit browser, go to the playground and set the step to 1
  • Type in 5.99999999999 into the input and confirm the error icon still remains

General checklist

  • Browser QA
    • Checked in Chrome, Safari, Edge, and Firefox
    • Checked for accessibility including keyboard-only and screenreader modes
  • Docs site QA
    • Skipped all docs steps, as the prop docs for step feels sufficient/accurate to the new behavior - LMK if you'd rather see more docs
  • Code quality checklist
  • Release checklist
    • A changelog entry exists and is marked appropriately.

@cee-chen
Add initial internal isStepInvalid state/logic
544e705
@cee-chen
Write tests
0207229 
+ move DRY checks so both describe blocks can use them
@cee-chen
[cleanup] DRY out isInvalid logic
712b079 
+ `aria-invalid` undefined logic
@cee-chen
Add extra custom invalid message logic to EuiValidatableControl
f6cabeb 
+ add a type that allows the `number` input to show the browser step invalid error message

- Otherwise, the input just unhelpfully shows 'Invalid' instead

+ Add missing unit tests for `useSetControlValidity`
@cee-chen
Fix/add catch for bizarre browser behavior
629f34c 
- If an `input type="number" is instantiated with a mismatched `step` and `value/defaultValue`, the input acts SUPER erratically at that point and browser validation can't be reconciled. The best we can do is warn and stop checking
@cee-chen
Update snapshots + remove mocks
5b4109f
@cee-chen cee-chen mentioned this pull request Sep 19, 2023
Closed
@kibanamachine
Copy link

Preview staging links for this PR:

@elasticmachine
Copy link
Collaborator

💚 Build Succeeded

@cee-chen
Copy link
Member Author

🤦 Closing out this PR as it doesn't fully address the integer rounding scenario in #7180 (in fact, nothing does and can, because this is browser/JS behavior)

@cee-chen cee-chen closed this Sep 19, 2023
@cee-chen cee-chen deleted the field-number-step branch September 19, 2023 23:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[EuiFieldNumber] Invalid sign disappear for non-integer value after 7 digits
3 participants
@cee-chen @kibanamachine @elasticmachine