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

task(settings): Display password requirements inline #17605

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Conversation

dschom
Copy link
Contributor

@dschom dschom commented Sep 13, 2024

Because

  • We are improving UX
  • Balloon UI pattern were problematic on mobile

This pull request

  • Adds new components that display password criteria inline
  • Removes info about passwords and encryption

Issue that this pull request solves

Closes: FXA-7896

Checklist

Put an x in the boxes that apply

  • My commit is GPG signed.
  • If applicable, I have modified or added tests which pass locally.
  • I have added necessary documentation (if appropriate).
  • I have verified that my changes render correctly in RTL (if appropriate).

Screenshots (Optional)

image

RTL
image

Other information (Optional)

The component with the 'balloon' UI pattern was shared by several pages. So in attempt do what this tickets asks, I just applied these changes to the password reset page; however, they could easily be applied to other pages such as the sign up page.

The code is largely the same as the FormPasswordWithBalloon. Notable differences involve onChange interactions. These are hard to specify in Figma, and when manually testing the component, I realized on change events tied to key presses could easily create a very jumpy user experience. This was not a problem with the balloon UI pattern, because it didn't actually alter the layout, where as the inline criteria and warnings, when displayed, will push text boxes downward. Please take an extra look / test of this and make sure it's acceptable. I can see some of this being subject to opinion.

@dschom dschom requested review from a team as code owners September 13, 2024 18:31
@vpomerleau
Copy link
Contributor

Just responding to the additional comments about the jumpy experience for now - @LZoog had brought this up in design reviews a long time ago, and UX team at the time had said the experience would be okay. Seeing it in action and with different team members on board now the decision might be different.

At some point there had been exploration done on removing the confirm password field but that option had been tabled - if we wanted to revisit this, we could consider having the password requirements always shown and the "passwords do not match" error would not come up. From what I recall, there is some research in the UX industry showing that the password confirmation field is not needed if the user has the option of viewing their entered password.

Copy link
Contributor

@vbudhram vbudhram left a comment

Choose a reason for hiding this comment

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

@dschom I dig it! I tested this manually and it works well 👍🏽

@@ -0,0 +1,20 @@
## FormPasswordInline
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be FormPasswordInlineCriteria?


import React from 'react';
import { useForm } from 'react-hook-form';
import FormPasswordInline, { PasswordFormType } from '.';
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: Looks like this exports FormPasswordWithInlineCriteria

decorators: [withLocalization],
} as Meta;

export const ResetPassword = () => (
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it possible to show the error state of passwords not matching in storybook?

Because:
- We are improving UX
- Balloon UI pattern were problematic on mobile

This Commit:
- Adds new components that dispay password criteria inline
- Removes info about passwords and encryption
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.

4 participants