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

Update two factor options layout #623

Merged
merged 5 commits into from
Sep 17, 2024

Conversation

thrijith
Copy link
Contributor

  • 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 #622

Why?

To make sure the UI consistent, as mentioned in #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

Screenshots or screencast

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

Changelog Entry

Changed - Two Factor Options layout

Align two factor options with title
class-two-factor-core.php Outdated Show resolved Hide resolved
@jeffpaul jeffpaul added this to the 0.10.0 milestone Aug 20, 2024
@thrijith thrijith requested a review from dd32 August 20, 2024 18:01
@thrijith
Copy link
Contributor Author

Does the asset screenshot need an update as well now?

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

@dd32
Copy link
Member

dd32 commented Aug 22, 2024

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
Copy link
Member

dd32 commented Aug 22, 2024

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

@thrijith
Copy link
Contributor Author

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

@jeffpaul
Copy link
Member

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

Copy link
Collaborator

@kasparsd kasparsd left a comment

Choose a reason for hiding this comment

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

This looks good -- safe to merge and can be included in the next point release.

I wonder if any third-party integrations relying on this table are somehow impacted by this. I think WP VIP was adding an SMS option in there, right?

@dd32
Copy link
Member

dd32 commented Sep 17, 2024

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 dd32 merged commit 71b4931 into WordPress:master Sep 17, 2024
24 checks passed
@kasparsd kasparsd mentioned this pull request Sep 18, 2024
@jeffpaul jeffpaul modified the milestones: 0.11.0, 0.10.0 Dec 2, 2024
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.

Replace table tags with div tags to match Application Passwords styles
4 participants