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

Updated to QR code page #1083

Merged
merged 1 commit into from
Jan 5, 2022
Merged

Updated to QR code page #1083

merged 1 commit into from
Jan 5, 2022

Conversation

ErikCH
Copy link
Contributor

@ErikCH ErikCH commented Jan 4, 2022

Issue #, if available:
#881

Description of changes:
Updated QR code page so users can copy the secret key, instead of using QR code.

setuptotp.mp4

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@aws-amplify-us-east-2
Copy link

This pull request is automatically being deployed by Amplify Hosting (learn more).

Access this pull request here: https://pr-1083.d3inl0muob87le.amplifyapp.com

@changeset-bot
Copy link

changeset-bot bot commented Jan 4, 2022

🦋 Changeset detected

Latest commit: bed81c1

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 3 packages
Name Type
@aws-amplify/ui-angular Patch
@aws-amplify/ui-react Patch
@aws-amplify/ui-vue Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@lgtm-com
Copy link

lgtm-com bot commented Jan 4, 2022

This pull request introduces 1 alert when merging bed81c1 into a4a1e21 - view on LGTM.com

new alerts:

  • 1 for Unused variable, import, function or class

@ErikCH ErikCH temporarily deployed to ci January 4, 2022 22:42 Inactive
@ErikCH ErikCH temporarily deployed to ci January 4, 2022 22:42 Inactive
@ErikCH ErikCH temporarily deployed to ci January 4, 2022 22:42 Inactive
@ErikCH ErikCH temporarily deployed to ci January 4, 2022 22:42 Inactive
Comment on lines +22 to +28
let secretKey = ref('');
let copyTextLabel = ref(translate('COPY'));

function copyText() {
navigator.clipboard.writeText(secretKey.value);
copyTextLabel.value = translate('COPIED');
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was the only change, just moved script setup to top.

Comment on lines +149 to +151
@media (max-width: 30rem) {
word-break: break-all;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

What do you think about making [data-amplify-copy] hidden if not on mobile?

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 dunno, I was just using a QR code the other day, and I appreciated how the secret key was also displayed. I don't think it has to be just for mobile.

Copy link
Contributor

Choose a reason for hiding this comment

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

That's fair! My only concern is that end-users won't know what to do with that secret key, but customers can use header slots to add directions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I was debating back and forth if I should add in some instructions on how to add 2FA to Authy, or Google Authenticator, but I didn't want to add more requirements unless a customer wanted too.

And they can add in their own with a slot, so you're right.

@ErikCH ErikCH requested a review from wlee221 January 5, 2022 16:26
Copy link
Contributor

@wlee221 wlee221 left a comment

Choose a reason for hiding this comment

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

🚢

@ErikCH ErikCH merged commit 2e7dbae into main Jan 5, 2022
@ErikCH ErikCH deleted the update-QR-code-page-secret branch January 5, 2022 18:57
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