Skip to content

feat(settings): new FlowSetup2faBackupCodeDownload component#18941

Merged
MagentaManifold merged 1 commit intomainfrom
FXA-11624
Jun 3, 2025
Merged

feat(settings): new FlowSetup2faBackupCodeDownload component#18941
MagentaManifold merged 1 commit intomainfrom
FXA-11624

Conversation

@MagentaManifold
Copy link
Contributor

Because

  • The design for 2FA setup has been updated
  • We want the UI component to be reusable
  • We want to document the component and states in storybook

This pull request

  • Creates a new UI component with l10n, storybook and unit tests
  • Tweaks Datablock to match the lastest design
  • Does not hook up the component in the setup flow - that will be done in a later ticket

Issue that this pull request solves

Closes: FXA-11624

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

Other information (Optional)

Any other information that is important to this pull request.

@MagentaManifold MagentaManifold marked this pull request as ready for review May 30, 2025 19:31
@MagentaManifold MagentaManifold requested review from a team as code owners May 30, 2025 19:31
Copy link
Contributor

@vpomerleau vpomerleau left a comment

Choose a reason for hiding this comment

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

Great work! Comments are small nits/suggestions for improvements. Not blocking!

@MagentaManifold MagentaManifold force-pushed the FXA-11624 branch 2 times, most recently from f2e6f02 to 9f8abe0 Compare June 2, 2025 17:50
@MagentaManifold MagentaManifold changed the title feat(settings): new FlowSetup2faBackupCodeDownLoad component feat(settings): new FlowSetup2faBackupCodeDownload component Jun 2, 2025
@MagentaManifold
Copy link
Contributor Author

I implemented the logic for UA detection inside the FlowSetup2faBackupCodeDownload component instead of passing it in as a prop, because I don't feel the need to pass it through so many layers of components, since this is the only component relying on the UA anyway.

Copy link
Contributor

@bcolsson bcolsson left a comment

Choose a reason for hiding this comment

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

Looks good, some minor suggestions (try to avoid acronyms in descriptions since those aren't easily understood by localizers).

Also adding a ending group comment as insurance due to quirks related to the string extract process.

Because:

* The design for 2FA setup has been updated
* We want the UI component to be reusable
* We want to document the component and states in storybook

This commit:

* Creates a new UI component with l10n, storybook and unit tests
* Tweaks Datablock to match the lastest design
* Does not hook up the component in the setup flow - that will be done in a later ticket

Closes #FXA-11624
Copy link
Contributor

@vpomerleau vpomerleau left a comment

Choose a reason for hiding this comment

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

Thanks for the updates! Approving with a small recommendation for the ua check.

onContinue,
reason = GleanClickEventType2FA.setup,
}: FlowSetup2faBackupCodeDownloadProps) => {
const [isMobile, setIsMobile] = React.useState(false);
Copy link
Contributor

Choose a reason for hiding this comment

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

Why set as state? This should only need to be computed once, since it is static (vs checking based on viewport sizing which could change). Perhaps useMemo or a plain const would be sufficient?

If updating this - it might also be good to update the comment with a tad more detail about what types will be reported as 'mobile' here vs having no known "type" (≈ desktop)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Checking UA is impure, so useMemo is not correct here. Using a plain const will cause the UA check to run every re-render. So I am using a combination of useState and useEffect, so that the check is only run once (on mount).

Will update in future PRs if better solution exists

@MagentaManifold MagentaManifold merged commit d72d088 into main Jun 3, 2025
19 checks passed
@MagentaManifold MagentaManifold deleted the FXA-11624 branch June 3, 2025 18:00
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.

3 participants