feat(account-settings): Add back SetupTOTP
#3149
Conversation
|
| export const ConfirmationCode: TextFieldComponent = ({ label, ...rest }) => ( | ||
| <TextField {...rest} label={label} /> | ||
| ); |
There was a problem hiding this comment.
This is one weird part that I need TS advice on. Instead of directly assigning this like
ConfirmationCode: TextFieldComponent = TextField`I had to separate out label to go around the error label is required but is optional in TextFieldComponent. I wanted to keep label optional because it's a UI prop and IMO shouldn't be required for override components.
There was a problem hiding this comment.
I tried experimenting with Omit and adding default params, but I couldn't get it to work. I think the way you did it is fine. Maybe @calebpollman has an idea?
There was a problem hiding this comment.
My concern with this approach is that a future maintainer might be confused if they were to attempt to update ConfirmationCode. Tbh not sure if i feel that we need to make label optional since requiring it is somewhat of a guard to ensure accessibility if i remember correctly. Also should this be ConfirmationCodeField?
| @@ -1,4 +1,5 @@ | |||
| export enum ComponentClassName { | |||
There was a problem hiding this comment.
Not related to this PR, but enum types should ideally be included in types files
packages/react/src/components/AccountSettings/SetupTOTP/types.ts
Outdated
Show resolved
Hide resolved
| export const SecretKeyQRCode: ImageComponent = Image; | ||
|
|
||
| export const CopySecretKey: ButtonComponent = Button; |
There was a problem hiding this comment.
What do you think of aligning the names of these closer to their primitives? Example:
const QRCodeImage;
const CopyButton;
| export const ConfirmationCode: TextFieldComponent = ({ label, ...rest }) => ( | ||
| <TextField {...rest} label={label} /> | ||
| ); |
There was a problem hiding this comment.
My concern with this approach is that a future maintainer might be confused if they were to attempt to update ConfirmationCode. Tbh not sure if i feel that we need to make label optional since requiring it is somewhat of a guard to ensure accessibility if i remember correctly. Also should this be ConfirmationCodeField?
| @@ -0,0 +1,5 @@ | |||
| const QR_CODE_SIZE = '228px'; | |||
There was a problem hiding this comment.
Does hard coding work well with mobile?
There was a problem hiding this comment.
I tried it with chrome + its mobile views, it worked okay:
(I took the hardcode width from Authenticator)
I'm open to improvement ideas tho. I'll play around with it too!
packages/react/src/components/AccountSettings/SetupTOTP/SetupTOTP.tsx
Outdated
Show resolved
Hide resolved
| ); | ||
|
|
||
| React.useEffect(() => { | ||
| if (user && !hasInit.current) { |
There was a problem hiding this comment.
Think we discussed this in the past, but can we just look at whether totpSecret is null here?
There was a problem hiding this comment.
user has to be there because user is initially null. I think I introduced hasInit because of some remounting issue with parent component.
Let me try using !totpSecret check again. If it turns out to be problematic later when we use this component in ConfigureMFA, we can revert it.
|
|
||
| setVerifyTotpStatus({ isVerifying: false, errorMessage: null }); | ||
|
|
||
| onSuccess?.(); // notify success to the parent |
There was a problem hiding this comment.
Think we can remove the comments here an on line 102 since onSuccess and onError are optional props and potentially custom callbacks (if i understand correctly)
| navigator.clipboard.writeText(totpSecret.secretKey); | ||
| }, [totpSecret]); | ||
|
|
||
| /* Return null if Auth.getCurrentAuthenticatedUser is still in progress */ |
There was a problem hiding this comment.
Nit: prefer inline syntax for comments that do not benefit from intellisense
| onSuccess, | ||
| onError, | ||
| }: ConfigureTOTPProps): JSX.Element | null { | ||
| const [formValues, setFormValues] = React.useState<FormValues>({ code: '' }); |
There was a problem hiding this comment.
Unless we plan to add more keys to formValues in the immediate future that this can be simplified to:
const [confirmationCode, setConfirmationCode] = React.useState('');
Co-authored-by: Caleb Pollman <cpollman@amazon.com>
…b.com:aws-amplify/amplify-ui into account-settings/revert-revert-configure-totp
…OTP.tsx Co-authored-by: Caleb Pollman <cpollman@amazon.com>
…b.com:aws-amplify/amplify-ui into account-settings/revert-revert-configure-totp
Description of changes
AccountSettings.SetupTOTPwas previously descoped out of preview release. Adding this back. The code was reviewed in #2969, but there are few updates:ConfigureTOTP->AccountSettings.SetupTOTPIssue #, if available
Description of how you validated changes
Unit tests
Checklist
yarn testpassesBy submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.