two-factor icon indicating copy to clipboard operation
two-factor copied to clipboard

Update two factor options layout

Open thrijith opened this issue 1 year ago • 3 comments

  • Update and move two factor option heading out of table
  • Align two factor options with title

What?

This PR updates the layout of Two Factor options to match Application Passwords, fixes https://github.com/WordPress/two-factor/issues/622

Why?

To make sure the UI consistent, as mentioned in https://github.com/WordPress/two-factor/issues/622, the request in it is to use div tags but we can't do it as fieldset tag is required to make all inputs disabled on a condition, which won't work with div without the some additional CSS.

How?

  • It moves the Two-Factor Options out of the table and adds a heading for it, adds some style to align the two factor options table with heading.

Testing Instructions

  • Start the env after checking out into the branch go to http://localhost:8888/wp-admin/profile.php, scroll down two factor options, confirm it matches the Application Passwords.

Screenshots or screencast

CleanShot 2024-08-16 at 17 33 31@2x

Changelog Entry

Changed - Two Factor Options layout

thrijith avatar Aug 16 '24 12:08 thrijith

Does the asset screenshot need an update as well now?

https://github.com/WordPress/two-factor/blob/master/assets/screenshot-1.png

thrijith avatar Aug 21 '24 12:08 thrijith

Does the asset screenshot need an update as well now?

It does, but the next release of the plugin will likely need to change other aspects of that page, so I'm not sure it's a priority for this PR.

Perhaps create a new issue to track that?

dd32 avatar Aug 22 '24 03:08 dd32

I've pushed some commits that makes the styles closer to core, by inheriting the core css via the expected classes, and adding the <tfoot> footer that other code tables do.

This has had some downsides though; as it's caused some UI issues at varying display sizes. Things that need work:

  • Mobile table layout
  • Table headers wrapping

@thrijith Apologies, this isn't all totally what you were expecting from this PR, but it seemed like to merge this, it would be better to cover all aspects of making it closer to the core UI.

Before After
Full width Screenshot 2024-08-22 at 2 33 45 PM Screenshot 2024-08-22 at 2 24 24 PM
Slightly less Screenshot 2024-08-22 at 2 33 56 PM Screenshot 2024-08-22 at 2 24 49 PM
Mobile Screenshot 2024-08-22 at 2 34 20 PM Screenshot 2024-08-22 at 2 25 00 PM

dd32 avatar Aug 22 '24 04:08 dd32

Hi @dd32, is there anything required from my end on this PR?

thrijith avatar Aug 30 '24 09:08 thrijith

@dd32 any further work needed on this PR or is this good to merge?

jeffpaul avatar Sep 16 '24 18:09 jeffpaul

Apologies, I didn't get back to this ticket.

Personally I'd prefer to resolve the mobile usability of it as above, but perhaps that can be pushed off to a new issue / PR, as it's arguably not worse than before. That might be better resolved via #342

This shouldn't cause any impacts for addon plugins, since there's no major underlying changes, but any that are expecting that specific HTML structure may break if they are using highly scoped CSS/JS selectors that don't account for the removal of the nested table.. which would be a poor choice IMHO.

Also noting, this will partially fix #536

dd32 avatar Sep 17 '24 01:09 dd32