WordPress / wporg-two-factor

2FA for WordPress.org accounts
37 stars 8 forks source link

Add a wizard component for first time configurations. #286

Closed StevenDufresne closed 3 months ago

StevenDufresne commented 4 months ago

It's likely that we enforce 2fa for plugins/theme committers on wp.org. This PR adds a wizard that we can show to users on login that will immediately get them into the configuration flow.

Main changes

Screenshots

Screenshot 2024-08-12 at 2 05 17 PM
Screenshot 2024-08-12 at 2 05 23 PM
Screenshot 2024-08-12 at 2 06 00 PM
Screenshot 2024-08-12 at 2 06 15 PM

How to test

Without local modifications to the underlying WP 2fa plugins, you can't test this. This is best tested in a sandbox.

  1. Apply code changes.
  2. Navigate to https://profiles.wordpress.org/me/profile/edit/group/3/?first-time=true
  3. Try configuring 2fa

Note, if you use an existing account, the wizard will show the "configured states" which isn't expected for regular users.

adamwoodnz commented 3 months ago

Using the tab key I can still navigate around behind the wizard. Probably need to trap the tab navigation within.

adamwoodnz commented 3 months ago

Generally we use sentence case for titles, and I can see a lot of title case. The bit that sticks out for me is the title below the logo.

Overall I think this could do with some design input; as well as the copy the progress bar details could be refined a little (line thickness, icons).

adamwoodnz commented 3 months ago

These back arrows are still using the old blue

Screenshot 2024-08-12 at 12 22 14 PM

StevenDufresne commented 3 months ago

Generally we use sentence case for titles, and I can see a lot of title case. The bit that sticks out for me is the title below the logo.

I've lowercased that option titles. The main titles are existing and enforced using CSS. I think we can address that later if needed.

StevenDufresne commented 3 months ago

These back arrows are still using the old blue

I've updated this here: https://github.com/WordPress/wporg-two-factor/commit/06f3e14dd2db2f51dd0cc724788f62ec5b5fb82e

adamwoodnz commented 3 months ago

Backup codes print view could do with some elements being hidden

adamwoodnz commented 3 months ago

This might be an existing issue but the back button and title don't fit

adamwoodnz commented 3 months ago

And the security key screen is collapsing

adamwoodnz commented 3 months ago

The outline focus style on these radio elements should be removed imo. The focus ring around the whole option and color change on the input should be enough.

jasmussen commented 3 months ago

Nice, looks good! Appreciate you using the new typography, colors, and componentry to build this out.

Overall, I wouldn't block this on any one particular piece, but a few things could be refined a bit. In no particular order:

Screenshot 2024-08-12 at 12 42 58

We should establish and adhere to a UI text-case guide. I think one exists, but the link to it escapes me. If I'm not wrong, it's sentence case with capitals for only proper nouns. In this case, it would be "Choose an authentication method", and probably "Configure two-factor", unless "two-factor" counts as a proper noun?

Screenshot 2024-08-12 at 12 45 43

Are the icons scaled down? With few exceptions we should only use those icons in 24x24px. In this case you might also want to replace the icons with numbers, 1, 2, 3 instead, as the icons themselves are not perfect.

Can you apply a 1.5px stroke-width both for the horizontal bar, and for the inner stroke of the circle?

Screenshot 2024-08-12 at 12 49 19

We can refine the Back button a bit: no underline, 24x24 small-chevron icon, same blueberry color. This is similar to what we did for older 2fa mockups.

security key

Screenshot 2024-08-12 at 12 50 50

Do we have yet a new notice component based on these designs from @fcoveram?

Screenshot 2024-08-12 at 12 51 44

If not, I would simply make the text bold and not box it up in that yelllow notice piece. It's an old icon, and old colors, and there's a checkbox below to make sure you understand.

Screenshot 2024-08-12 at 12 52 49

Can these buttons be right below the codes themseles?

Screenshot 2024-08-12 at 12 53 48

Instead of "All Finished", should it be "Finish" or "Complete", or even "I understand"? Can it be a disabled solid button instead of a disabled outline button? I.e. this:

Screenshot 2024-08-12 at 12 55 04

Screenshot 2024-08-12 at 12 55 13

Can we omit "Setup Complete" here? Since there's no background, the centered heading becomes a bit off-balanced given there's also a heading right below.

I'd also tend to omit the emoji here. If we want something celebratory, we could potentially revisit the animation we explored for the past mockups:

animation
StevenDufresne commented 3 months ago

Do we have yet a new notice component based on these designs from @fcoveram?

This plugin was built using wordpress/components so we don't have access to wp.org's notices. We could manually restyle it.

I wonder if we can remove it in favor of hiding the "back" button when codes are generated that need saving and updating the copy to include some of this language?

StevenDufresne commented 3 months ago

Can it be a disabled solid button instead of a disabled outline button? I.e. this:

It can, but if we do, lets do that for all the other buttons as well. Can we open a new ticket for that?

StevenDufresne commented 3 months ago

Can we omit "Setup Complete" here? Since there's no background, the centered heading becomes a bit off-balanced given there's also a heading right below.

It's probably easier to remove in the header in the content than re-working the component title. Its probably not as important.

jasmussen commented 3 months ago

It can, but if we do, lets do that for all the other buttons as well. Can we open a new ticket for that?

Just to understand, the button component by default has an outline-look when disabled, regardless of its base variant? If yes, I'll open an issue, I'd expect primary buttons to remain primary-looking when disabled.

StevenDufresne commented 3 months ago

Just to understand, the button component by default has an outline-look when disabled, regardless of its base variant? If yes, I'll open an issue, I'd expect primary buttons to remain primary-looking when disabled.

We have a few use cases in different components where the button defaults to disabled, we'll want to keep those consistent whether the button is outlined or filled in.

jasmussen commented 3 months ago

Exactly, if it's filled, it should always be filled (and have a disabled state), etc. It sounds like that's the case here, so why's it outlined if it's primary when not-disabled? Forgive me if I still miss nuance.

Past experience tells me the best disabled state is just an opacity change (e.g. opacity: 0.3), rather than a bespoke visual style.

StevenDufresne commented 3 months ago

Closing, we'll go with an inline version: https://github.com/WordPress/wporg-two-factor/pull/297