Navigation Menu

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

feat(payments): rework form validation to handle focus & blur independently #3027

Merged
merged 1 commit into from Oct 18, 2019
Merged

feat(payments): rework form validation to handle focus & blur independently #3027

merged 1 commit into from Oct 18, 2019

Conversation

lmorchard
Copy link
Contributor

  • rework onValidate handlers in form fields - they work on both focused
    and blurred states, can supply valid state and error message separately

  • remove most validation logic out of field components and into default
    validation handler functions

  • test updates to handle new arrangement

  • misc small bug and lint fixes

  • squelch expected exceptions during AppErrorBoundary test

  • add exception for NPM security advisory for https://npmjs.com/advisories/1184

fixes #2994

@lmorchard
Copy link
Contributor Author

@johngruen Okay, I think this might address some of the issues we're been hitting on this form over time. Field contents get validated on change, but tooltips only get displayed on blur. That means the submit button should enable / disable as expected on form changes, but tooltips should be less noisy and not appear until the user moves away from the field.

@lmorchard
Copy link
Contributor Author

Also, this can probably most easily get tested via storybook:

npm run storybook, visit http://localhost:6006/?path=/story/components-paymentform--default

@@ -1,3 +1,5 @@
{
"exceptions": []
"exceptions": [
"https://npmjs.com/advisories/1184"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unrelated, but it came up today as a basically harmless advisory for our builds, so I figured I'd throw this in

@@ -10,6 +10,17 @@ import { AppContext } from './lib/AppContext';
// describe('App', () => {});

describe('App/AppErrorBoundary', () => {
beforeEach(() => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also unrelated to this PR, but quiets a useless error log during tests

…dently

- rework onValidate handlers in form fields - they work on both focused
  and blurred states, can supply valid state and error message separately

- remove most validation logic out of field components and into default
  validation handler functions

- test updates to handle new arrangement

- misc small bug and lint fixes

- squelch expected exceptions during AppErrorBoundary test

- add exception for NPM security advisory for https://npmjs.com/advisories/1184

fixes #2994
Copy link

@meandavejustice meandavejustice left a comment

Choose a reason for hiding this comment

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

This is working great for me. Also thanks for adding the security exception and silencing that error. Hopefully we won't have to mess with this validation now for a while :)

@lmorchard lmorchard merged commit fe05ea9 into mozilla:master Oct 18, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Deleting the contents of some fields of a Subscription page doesn't disable the Submit button
2 participants