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

Simplify the Two Factor settings in user profile #654

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

kasparsd
Copy link
Collaborator

@kasparsd kasparsd commented Dec 3, 2024

Fixes #342.

With the recent changes to the admin UX in #623, we have the opportunity to simplify it even further and make it look more native to the core WP settings.

What?

Switch to a standard two column layout with section headings on the right:

two-factor-ux-proposal

Why?

  1. Using standard layout improves the user experience and accessibility of the settings section.
  2. Native-looking layout would help with the core project merge if it gets to it.

How?

This proposal relies on the core idea that the primary method selector can be moved to a dedicated dropdown instead of using the radio buttons next to each method configuration.

Importantly:

  1. The primary method selector is a pure convenience thing as the user can switch between all of the enabled methods during the login phase anyway. The plugin could show the most secure option, by default, if no primary is specified.

  2. Secondly, the primary method doesn't need to be configured if the user has only one two factor method enabled (and the backup codes, for example). The plugin can be smart enough to guess the primary method.

Considerations:

  • The primary method selector is placed at the end of all individual methods to ensure that users have evaluated all the options and make the choice, if really necessary for their use-case.

Testing Instructions

Screenshots or screencast

Changelog Entry

Added - New feature.
Changed - Existing functionality.
Deprecated - Soon-to-be removed feature.
Removed - Feature.
Fixed - Bug fix.
Security - Vulnerability.

@kasparsd kasparsd self-assigned this Dec 3, 2024
@kasparsd
Copy link
Collaborator Author

kasparsd commented Dec 3, 2024

Curious to hear what others think about this proposal.

@dd32 Do you have any concerns with this in regards to various integrations that insert themselves into this view?

Required for settings and to seperate from default pick by system
@jeffpaul jeffpaul added this to the 0.11.0 milestone Dec 3, 2024
@jeffpaul jeffpaul requested a review from dd32 December 3, 2024 14:36
@jeffpaul
Copy link
Member

jeffpaul commented Dec 3, 2024

Noting this change would close #342. I do like moving the Primary radio button out to a dropdown single select option, as that handles for most of the setup UX confusion for users.

It might make sense to group the Email / Auth app / U2F / Codes / Dummy, etc. bits into a "Two-factor methods" section so that the Primary Method setting appears more clearly separate to the method configurations.

Finally, a small tweak to the Primary Method helper copy from Select the primary method used during the login by default. to Select the primary method to use for two-factor authentication when signing into this site..

@kasparsd
Copy link
Collaborator Author

kasparsd commented Dec 3, 2024

Great suggestions @jeffpaul, thanks!

It might make sense to group ...

We currently have everything under "Two-Factor Options". Something like this?


Two-Factor

Configure a primary...

Two-Factor Methods

Method A - [ ] Enable Method A
Method B - [ ] Enable Method B
Method C - [ ] Enable Method C

Two-Factor Primary Method

Primary method dropdown.

@jeffpaul
Copy link
Member

jeffpaul commented Dec 3, 2024

Yes, adding those headings I think helps separate things just a bit and mimics how other WP core settings pages group things. I admit I was thinking more visually to have horizontal lines/separators but I think following WP core and using H2/H3/etc is probably the better/right approach.

@dd32
Copy link
Member

dd32 commented Dec 5, 2024

@dd32 Do you have any concerns with this in regards to various integrations that insert themselves into this view?

Not at all!

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.

Onboarding: checking both "enabled" and "primary" is confusing
3 participants