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

LG-12596: Update security key setup page #10323

Merged
merged 24 commits into from
Apr 4, 2024

Conversation

jc-gsa
Copy link
Contributor

@jc-gsa jc-gsa commented Mar 27, 2024

🎫 Ticket

LG-12596

🛠 Summary of changes

Updates the security key setup page found at /webauthn_setup.

changelog: User-Facing Improvements, Authentication, Update security key setup form
@jc-gsa jc-gsa changed the title Lg 12596 update security key setup LG-12596: Update security key setup Mar 27, 2024
@jc-gsa jc-gsa changed the title LG-12596: Update security key setup LG-12596: Update security key setup page Mar 27, 2024
@jc-gsa jc-gsa force-pushed the LG-12596-Update-security-key-setup branch from 7112e0b to 08a8657 Compare March 27, 2024 04:54
@jc-gsa jc-gsa force-pushed the LG-12596-Update-security-key-setup branch from 08a8657 to 202f4ff Compare March 27, 2024 05:08
<% end %>
<%= c.with_item(heading: t('forms.webauthn_setup.step_2')) do %>
<% if @mobile %>
<%= image_tag asset_url('mfa-options/security_key_mobile.gif'), class: 'rounded-xl', alt: t('forms.webauthn_setup.step_2_image_mobile_alt') %>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Open to different CSS classes for this style.

Copy link
Contributor Author

@jc-gsa jc-gsa left a comment

Choose a reason for hiding this comment

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

Double checking wether or not we want to remove the remember device checkbox. Designs do not seem to have it, yet I believe other pages like TOTP setup do.

@aduth
Copy link
Member

aduth commented Mar 27, 2024

Double checking wether or not we want to remove the remember device checkbox. Designs do not seem to have it, yet I believe other pages like TOTP setup do.

That's a good call-out, can you check to confirm with the UX team?

Personally I would prefer that we have it, since remembered device is a big usability boost, and I wouldn't feel comfortable defaulting to remember without the user's awareness.

@jc-gsa jc-gsa force-pushed the LG-12596-Update-security-key-setup branch from 9358beb to b723c56 Compare March 27, 2024 19:22
@jc-gsa jc-gsa requested a review from a team March 27, 2024 19:48
app/views/users/webauthn_setup/new.html.erb Outdated Show resolved Hide resolved
spec/support/features/session_helper.rb Outdated Show resolved Hide resolved
app/views/users/webauthn_setup/new.html.erb Show resolved Hide resolved
@jc-gsa jc-gsa force-pushed the LG-12596-Update-security-key-setup branch from 8c7df36 to 851f88d Compare March 28, 2024 19:20
@jc-gsa jc-gsa force-pushed the LG-12596-Update-security-key-setup branch from 7a5e979 to 5506917 Compare April 2, 2024 15:53
Copy link
Member

@aduth aduth left a comment

Choose a reason for hiding this comment

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

Couple minor comments about test coverage, but tested and LGTM otherwise 👍

app/controllers/users/webauthn_setup_controller.rb Outdated Show resolved Hide resolved
app/presenters/webauthn_setup_presenter.rb Show resolved Hide resolved
@jc-gsa jc-gsa merged commit 1b8ce13 into main Apr 4, 2024
2 checks passed
@jc-gsa jc-gsa deleted the LG-12596-Update-security-key-setup branch April 4, 2024 20:02
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.

None yet

5 participants