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(settings): Create frontend components for recovery phone setup from Settings #18159

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

vpomerleau
Copy link
Contributor

Because

  • We are adding a new recovery phone feature

This pull request

  • Create FlowSetupRecoveryPhoneConfirmCode component w/ l10n, stories, unit tests
  • Create FlowSetupRecoveryPhoneSubmitPhone component w/ l10n, stories, unit tests
  • Create PageRecoveryPhoneSetup component w/ l10n, stories, unit tests

Issue that this pull request solves

Closes: #FXA-10236, #FXA-10237, #FXA-10245

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)

Will add storybook links when ready

Other information (Optional)

Draft for storybook review and self-review

@vpomerleau vpomerleau force-pushed the FXA-10237 branch 5 times, most recently from 7ea797e to 159c664 Compare December 23, 2024 16:48
@vpomerleau vpomerleau marked this pull request as ready for review December 23, 2024 16:48
@vpomerleau vpomerleau requested review from a team as code owners December 23, 2024 16:48
Copy link
Contributor

@LZoog LZoog left a comment

Choose a reason for hiding this comment

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

LGTM with a few comments. 👍

</LinkExternal>
</span>
role={type === 'error' ? 'alert' : 'status'}
aria-live={type === 'error' ? 'assertive' : 'polite'}
Copy link
Contributor

Choose a reason for hiding this comment

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

-chef's kiss- Yep, just tried this out and I love that it's immediately read while not moving focus back up to the error.

I think in that follow up issue, one thing we could also add, is that we should be focusing input back to the form field that caused the problem. Thankfully our forms are very tiny and I think we can do this pretty easily by always putting focus on the first form field.

@@ -20,6 +20,7 @@ export type BannerProps = {
animation?: Animation;
dismissButton?: DismissButtonProps;
link?: BannerLinkProps;
isFancy?: boolean;
Copy link
Contributor

Choose a reason for hiding this comment

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

} as Meta;

const mockSubmit = async (phoneNumber: string) => {
action('submitPhoneNumber')(phoneNumber);
Copy link
Contributor

Choose a reason for hiding this comment

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

But really - I haven't played with this enough, seems cool!

<FtlMsg id="tfa-row-enabled-info-link">
<LinkExternal
href="https://support.mozilla.org/kb/secure-firefox-account-two-step-authentication"
className="link-blue text-sm "
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: extra space

Should we have a TODO here to updating the heading to the proper link when it's created?

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 think in this case the link is correct as is - this is a link to general information about two-step authentication. Where we use a similar link name specifically for backup recovery phone we could update the link to the section that directly covers that method.


type PageRecoveryPhoneSetupProps = {
testPhoneNumber?: string;
testStep?: 1 | 2;
Copy link
Contributor

Choose a reason for hiding this comment

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

Might add a comment that these prop names are temporary until implementation?

title={localizedPageTitle}
onBackButtonClick={navigateBackward}
>
<ProgressBar currentStep={1} numberOfSteps={2} />
Copy link
Contributor

Choose a reason for hiding this comment

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

Feels like maybe you could lift/share that numberOfSteps const you set with this somehow, since if that changes this also will change.

const clearBanners = async () => {
return new Promise<void>((resolve) => {
setLocalizedErrorBannerMessage(''); // Clear the banner message
setTimeout(() => resolve(), 0); // Wait for DOM update
Copy link
Contributor

Choose a reason for hiding this comment

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

Hm why do we need a setTimeout? I think this isn't great for performance. In other places, we simply clear it with ''.

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 was attempting to fix an issue with the banner message not always being announced or getting interrupted - unfortunately this wasn't 100% reliable so I will remove.

</FtlMsg>
<FtlMsg
id="flow-setup-phone-confirm-code-instruction"
elems={{ span: <span dir="ltr" className="font-bold"></span> }}
Copy link
Contributor

Choose a reason for hiding this comment

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

💎

)}
{localizedErrorBannerMessage && (
<Banner
ref={(el) => el && el.focus()}
Copy link
Contributor

Choose a reason for hiding this comment

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

Same question here as in another comment about this getting focus.

};

// Format the phone number as the user types - for easier review by the user
const formatPhoneNumber = (input: string): string => {
Copy link
Contributor

Choose a reason for hiding this comment

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

This does look nice! \o/

@vpomerleau vpomerleau force-pushed the FXA-10237 branch 2 times, most recently from 691d39d to c15f5e6 Compare December 24, 2024 20:42
…n settings

Because:

* We are adding a new recovery phone feature

This commit:

* Create FlowSetupRecoveryPhoneSubmitPhone component w/ l10n, stories, unit tests
* Create FlowSetupRecoveryPhoneConfirmCode component w/ l10n, stories, unit tests
* Create PageRecoveryPhoneSetup component w/ l10n, stories, unit tests
* Update Banner component to improve a11y (applied to new components) and add "fancy" info message style

Closes #FXA-10237
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.

2 participants