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

fix(fxa-settings): Ensure useWatch uses current input value as default #15436

Merged
merged 1 commit into from
Jun 15, 2023

Conversation

vpomerleau
Copy link
Contributor

Because

  • Controlled components that use useWatch to obtain the current input value would reset to default (empty string) on submit or re-render
  • We want to ensure that if the submit aborts due to an error or if the page re-renders, controlled components will display the current input value and not an empty string.

This pull request

  • Use getValues to set the default input value for useWatch in both places where it is used in the codebase (FlowRecoveryKeyHint and ResetPassword)

Issue that this pull request solves

Closes: #FXA-7547

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)

Please attach the screenshots of the changes made in case of change in user interface.

Other information (Optional)

Any other information that is important to this pull request.

@vpomerleau vpomerleau requested a review from a team as a code owner June 12, 2023 20:55
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.

One question/possible slight improvement but LGTM + confirmed to fix the bug.

@@ -130,7 +130,7 @@ export const FlowRecoveryKeyHint = ({
const hint: string = useWatch({
control,
name: 'hint',
defaultValue: '',
defaultValue: getValues().hint,
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure if it's possible for this to end up as undefined but I think it might? Feels "safer" to me to go with getValues().hint || '' to ensure it's always a string and same with the other reference, but maybe it errors higher up if that's the case.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

'hint' has a default value set on L51, which should prevent this from being undefined

Because:

* Controlled components that used useWatch to obtain the current input value would reset to default (empty string) on submit or rerender
* We want to ensure that if the submit aborts due to an error or if the page rerenders, controlled components will display the current input value and not an string.

This commit:

* Use getValues to set the default input value for useWatch in both places where it is used in the codebase (FlowRecoveryKeyHint and ResetPassword)

Closes #FXA-7547
@vpomerleau vpomerleau merged commit 4173969 into main Jun 15, 2023
@vpomerleau vpomerleau deleted the FXA-7547 branch June 15, 2023 22:20
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