Improve Confirm Password Error Validation #858

ErikCH · 2021-11-23T21:41:10Z

Issue #, if available:

Description of changes:
Improved UX for confirm. Now it shows the error on blur, or when the value is over 6 characters

Validation occurs on blur (Less then six characters)

sign-up-error-on-blur.mp4

Validation occurs after six or more characters are type

sign-up-error-character-limit.mp4

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

changeset-bot · 2021-11-23T21:41:12Z

🦋 Changeset detected

Latest commit: 86533e8

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 4 packages

Name	Type
@aws-amplify/ui-angular	Patch
@aws-amplify/ui-react	Patch
@aws-amplify/ui	Patch
@aws-amplify/ui-vue	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

ErikCH · 2021-11-23T21:42:00Z

...ts/ui-angular/src/lib/components/authenticator/components/form-field/form-field.component.ts

+  public onBlur($event: Event) {
+    let { name } = <HTMLInputElement>$event.target;
+
+    this.authenticator.updateForm({ name: `touched_${name}`, value: true });


Now we can track the touched elements in the form with the name touched_password or touched_confirm_password

ErikCH · 2021-11-23T21:42:33Z

...es/angular/projects/ui-angular/src/lib/primitives/password-field/password-field.component.ts

@@ -16,6 +16,7 @@ export class PasswordFieldComponent {
  @Input() placeholder = '';
  @Input() required = true;
  @Input() labelHidden = false;
+  @Output() onBlur = new EventEmitter<Event>();


This could have been an Input() or Output(). I felt Output worked here so I left it

ErikCH · 2021-11-23T21:42:56Z

packages/e2e/cypress/integration/common/sign-in.ts

@@ -33,5 +33,5 @@ When(
 );

 When('I type my password', () => {
-  cy.findInputField('Password').type(Cypress.env('VALID_PASSWORD'));
+  cy.findInputField('Password').type(Cypress.env('VALID_PASSWORD')).blur();


Tests were failing unless I added blur here.

packages/ui/src/machines/authenticator/defaultServices.ts

lgtm-com · 2021-11-23T21:50:31Z

This pull request fixes 7 alerts when merging 6c4da2f into b555532 - view on LGTM.com

fixed alerts:

7 for Unused variable, import, function or class

lgtm-com · 2021-11-23T22:16:05Z

This pull request fixes 7 alerts when merging 24331fb into b555532 - view on LGTM.com

fixed alerts:

7 for Unused variable, import, function or class

ErikCH · 2021-11-23T22:17:27Z

.../ui-angular/src/lib/components/authenticator/components/form-field/form-field.component.html

@@ -24,6 +24,7 @@
    [disabled]="disabled"
    [labelHidden]="labelHidden"
    [autocomplete]="inferAutocomplete()"
+    (setBlur)="onBlur($event)"


Angular linter complained here if I called this blur or onblur. So I changed the name to setBlur

ErikCH · 2021-11-23T22:19:16Z

The only UX I don't love here is if the user types in a password that's less then six characters long, and it matches the confirm password. An error won't display until they click somewhere else on the form. So they could possibly click the Create Account but and then they'll receive the error. I think this is fine until we look at adding in proper password validation from Zero config.

lgtm-com · 2021-11-24T01:13:27Z

This pull request fixes 7 alerts when merging 8f8f118 into b15f97e - view on LGTM.com

fixed alerts:

7 for Unused variable, import, function or class

ErikCH · 2021-11-24T01:53:29Z

packages/e2e/cypress/integration/common/sign-up.ts

+  cy.findInputField('Confirm Password')
+    .type(Cypress.env('VALID_PASSWORD'))
+    .blur()
+    .wait(100);
 });


There is something odd timing wise, that I cannot duplicate in real life only in cypress. Where if you send the blur at the exact same time as you click the acknowledgement button it causes the validation not to fire and the button won't light up. That's why I added a wait in here. Not sure if there is a better way of handling the blur...

Weird, we can keep an eye on it. Fortunately (and unfortunately) this isn't human-reproducible, so I'm good with the changes!

ericclemmons

I wanted to review this before the holidays because forms have been on my mind, particularly because of React Native support.

I think this PR has shown where we've outgrown our simplistic formData structure and need meta data to support that complexity like https://formik.org/docs/overview#the-gist does:

Validate entire form on submit
Validate single/multiple fields (e.g. confirm password) on blur
Validate single/multiple fields on change
Accessing touched.* meta data
Accessing errors.* meta data

My only actual hesitation with this PR is that touched_* properties become a public API that's accessible via formData when customizing a service. But also knowing that form validation is tricky & we'll want to track touched for most fields to perform more targeted validation.

ericclemmons · 2021-11-24T15:18:36Z

packages/ui/src/machines/authenticator/defaultServices.ts

+      touched_confirm_password,
+      touched_password,


I had a feeling we'd come across this need sooner than later. Libraries like Formik (https://formik.org/docs/overview#the-gist) track a touched object to maintain this state.

The dilemma I see here is that formData now introduces 2 extra values that weren't present before.

This likely isn't a problem because our signUp service has an allow-list of which values to send, but it seems like a smell to have a values for non-existent fields to track state, doesn't it?

Yeah that would make sense. Perhaps, we could add this in a future release? Seems like a big task

I agree that having touched context will be a desirable API on the long run, considering we'll look to document this for custom validators later on.

I personally think we should handle this from this PR to keep the API layers consistent. We can scope this to just the passwords -- I'll be happy to help too! (edit: I misread the code 😓 )

What if I create a new formControls context, that will have all the touched.* values, at least for right now just for password and confirm_password. We can build on this to add in more form validation for the future, and it will remove the values out of formValues into their own separate object. I'll push that up, so I can get your feedback.

Pushed in 032b48b @ericclemmons @wlee221 . Is this what you had in mind?

Awesome! One last nit -- what do you think about renaming formControls to just touched for now, and let the keys be ${name} instead of touched_${name}? Then when we need other formControls, we can expose them as separate facade. I think this separation of form facades will improve DX than having a general formControls, on the long run. What do you think @ericclemmons @ErikCH?

This will need some modification to runValidators shape though... 😓 but I think now is a good time to make changes, before we document them!

Sure! I updated the name to touched and update the validators 15c8b02 and 86533e8

packages/react/src/components/Authenticator/SignUp/FormFields.tsx

packages/ui/src/machines/authenticator/defaultServices.ts

…ssword too for all frameworks

packages/ui/src/helpers/auth.ts

packages/ui/src/machines/authenticator/actions.ts

packages/ui/src/machines/authenticator/defaultServices.ts

lgtm-com · 2021-11-24T21:09:57Z

This pull request introduces 1 alert and fixes 7 when merging 25c7dd6 into 2a486da - view on LGTM.com

new alerts:

1 for Unused variable, import, function or class

fixed alerts:

7 for Unused variable, import, function or class

wlee221

Thanks for the update, this is a big change! Sorry my review was late. Got few comments around tracking blurred inptus.

wlee221 · 2021-11-24T21:30:18Z

packages/e2e/cypress/integration/common/sign-up.ts

+  cy.findInputField('Confirm Password')
+    .type(Cypress.env('VALID_PASSWORD'))
+    .blur()
+    .wait(100);
 });


Weird, we can keep an eye on it. Fortunately (and unfortunately) this isn't human-reproducible, so I'm good with the changes!

wlee221 · 2021-11-24T21:59:25Z

packages/ui/src/machines/authenticator/defaultServices.ts

+      touched_confirm_password,
+      touched_password,


I agree that having touched context will be a desirable API on the long run, considering we'll look to document this for custom validators later on.

I personally think we should handle this from this PR to keep the API layers consistent. We can scope this to just the passwords -- I'll be happy to help too! (edit: I misread the code 😓 )

packages/ui/src/machines/authenticator/defaultServices.ts

lgtm-com · 2021-11-26T17:56:03Z

This pull request introduces 1 alert and fixes 7 when merging 032b48b into bb8906d - view on LGTM.com

new alerts:

1 for Unused variable, import, function or class

fixed alerts:

7 for Unused variable, import, function or class

wlee221

Apologies my reviews are getting late. Had one last nit about formControls 🙏

wlee221 · 2021-11-26T19:26:00Z

packages/ui/src/machines/authenticator/defaultServices.ts

+      touched_confirm_password,
+      touched_password,


Awesome! One last nit -- what do you think about renaming formControls to just touched for now, and let the keys be ${name} instead of touched_${name}? Then when we need other formControls, we can expose them as separate facade. I think this separation of form facades will improve DX than having a general formControls, on the long run. What do you think @ericclemmons @ErikCH?

This will need some modification to runValidators shape though... 😓 but I think now is a good time to make changes, before we document them!

lgtm-com · 2021-11-26T21:08:14Z

This pull request introduces 1 alert and fixes 7 when merging 86533e8 into a72d999 - view on LGTM.com

new alerts:

1 for Unused variable, import, function or class

fixed alerts:

7 for Unused variable, import, function or class

wlee221

LGTM, thanks @ErikCH!!

Added better confirm dx

6746ff4

ErikCH commented Nov 23, 2021

View reviewed changes

packages/ui/src/machines/authenticator/defaultServices.ts Outdated Show resolved Hide resolved

Fixed devtools

6c4da2f

Erik Hanchett added 2 commits November 23, 2021 13:58

Fixed output name

e655083

added changset

dc62d66

ErikCH changed the title ~~Improve Confirm Password DX~~ Improve Confirm Password Error Validation Nov 23, 2021

Removed private package from changeset

24331fb

ErikCH commented Nov 23, 2021

View reviewed changes

ErikCH had a problem deploying to ci November 23, 2021 22:19 Error

ErikCH had a problem deploying to ci November 23, 2021 22:19 Failure

added wait to fix tests

8f8f118

ErikCH temporarily deployed to ci November 24, 2021 01:15 Inactive

ErikCH temporarily deployed to ci November 24, 2021 01:34 Inactive

ErikCH commented Nov 24, 2021

View reviewed changes

ericclemmons reviewed Nov 24, 2021

View reviewed changes

Added new updateBlur facade, update for confirm password and reset pa…

25c7dd6

…ssword too for all frameworks

ErikCH commented Nov 24, 2021

View reviewed changes

packages/ui/src/helpers/auth.ts Show resolved Hide resolved

ErikCH commented Nov 24, 2021

View reviewed changes

packages/ui/src/machines/authenticator/actions.ts Outdated Show resolved Hide resolved

ErikCH commented Nov 24, 2021

View reviewed changes

packages/ui/src/machines/authenticator/defaultServices.ts Outdated Show resolved Hide resolved

ErikCH requested a review from ericclemmons November 24, 2021 21:09

ErikCH temporarily deployed to ci November 24, 2021 21:17 Inactive

ErikCH temporarily deployed to ci November 24, 2021 21:34 Inactive

wlee221 reviewed Nov 24, 2021

View reviewed changes

Added new formControls in context

032b48b

ErikCH requested a review from wlee221 November 26, 2021 17:51

ErikCH temporarily deployed to ci November 26, 2021 18:01 Inactive

ErikCH temporarily deployed to ci November 26, 2021 18:22 Inactive

wlee221 reviewed Nov 26, 2021

View reviewed changes

Erik Hanchett added 2 commits November 26, 2021 12:40

Updated named from formControls to touched

15c8b02

Got rid of console.logs

86533e8

ErikCH temporarily deployed to ci November 26, 2021 20:50 Inactive

ErikCH temporarily deployed to ci November 26, 2021 21:10 Inactive

wlee221 approved these changes Nov 26, 2021

View reviewed changes

ErikCH merged commit cc4a328 into main Nov 26, 2021

ErikCH deleted the confirm-ux branch November 26, 2021 21:22

github-actions bot mentioned this pull request Nov 26, 2021

Version Packages #893

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve Confirm Password Error Validation #858

Improve Confirm Password Error Validation #858

ErikCH commented Nov 23, 2021 •

edited

Loading

changeset-bot bot commented Nov 23, 2021 •

edited

Loading

ErikCH Nov 23, 2021

ErikCH Nov 23, 2021

ErikCH Nov 23, 2021

lgtm-com bot commented Nov 23, 2021

lgtm-com bot commented Nov 23, 2021

ErikCH Nov 23, 2021

ErikCH commented Nov 23, 2021

lgtm-com bot commented Nov 24, 2021

ErikCH Nov 24, 2021

wlee221 Nov 24, 2021 •

edited

Loading

ErikCH Nov 26, 2021

ericclemmons left a comment

ericclemmons Nov 24, 2021

ErikCH Nov 24, 2021

wlee221 Nov 24, 2021 •

edited

Loading

ErikCH Nov 26, 2021

ErikCH Nov 26, 2021 •

edited

Loading

wlee221 Nov 26, 2021

ErikCH Nov 26, 2021

lgtm-com bot commented Nov 24, 2021

wlee221 left a comment

wlee221 Nov 24, 2021 •

edited

Loading

wlee221 Nov 24, 2021 •

edited

Loading

lgtm-com bot commented Nov 26, 2021

wlee221 left a comment

wlee221 Nov 26, 2021

lgtm-com bot commented Nov 26, 2021

wlee221 left a comment

Improve Confirm Password Error Validation #858

Improve Confirm Password Error Validation #858

Conversation

ErikCH commented Nov 23, 2021 • edited Loading

changeset-bot bot commented Nov 23, 2021 • edited Loading

🦋 Changeset detected

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lgtm-com bot commented Nov 23, 2021

lgtm-com bot commented Nov 23, 2021

Choose a reason for hiding this comment

ErikCH commented Nov 23, 2021

lgtm-com bot commented Nov 24, 2021

Choose a reason for hiding this comment

wlee221 Nov 24, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ericclemmons left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wlee221 Nov 24, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ErikCH Nov 26, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lgtm-com bot commented Nov 24, 2021

wlee221 left a comment

Choose a reason for hiding this comment

wlee221 Nov 24, 2021 • edited Loading

Choose a reason for hiding this comment

wlee221 Nov 24, 2021 • edited Loading

Choose a reason for hiding this comment

lgtm-com bot commented Nov 26, 2021

wlee221 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lgtm-com bot commented Nov 26, 2021

wlee221 left a comment

Choose a reason for hiding this comment

ErikCH commented Nov 23, 2021 •

edited

Loading

changeset-bot bot commented Nov 23, 2021 •

edited

Loading

wlee221 Nov 24, 2021 •

edited

Loading

wlee221 Nov 24, 2021 •

edited

Loading

ErikCH Nov 26, 2021 •

edited

Loading

wlee221 Nov 24, 2021 •

edited

Loading

wlee221 Nov 24, 2021 •

edited

Loading