two-factor
two-factor copied to clipboard
Update two factor options layout
- 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 Optionsout 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
Changelog Entry
Changed - Two Factor Options layout
Does the asset screenshot need an update as well now?
https://github.com/WordPress/two-factor/blob/master/assets/screenshot-1.png
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?
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 | ||
| Slightly less | ||
| Mobile |
Hi @dd32, is there anything required from my end on this PR?
@dd32 any further work needed on this PR or is this good to merge?
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