Unify button focus styles in login, sign up and onboarding stepper nav #99693
Conversation
Jetpack Cloud live (direct link)
Automattic for Agencies live (direct link)
|
adamwoodnz
added
[Type] Bug
|
Here is how your PR affects size of JS and CSS bundles shipped to the user's browser: App Entrypoints (~107 bytes added 📈 [gzipped]) Common code that is always downloaded and parsed every time the app is loaded, no matter which route is used. Sections (~41953 bytes removed 📉 [gzipped]) Sections contain code specific for a given set of routes. Is downloaded and parsed only when a particular route is navigated to. Async-loaded Components (~359 bytes removed 📉 [gzipped]) React components that are loaded lazily, when a certain part of UI is displayed for the first time. Legend What is parsed and gzip size?Parsed Size: Uncompressed size of the JS and CSS files. This much code needs to be parsed and stored in memory. Generated by performance advisor bot at iscalypsofastyet.com. |
matticbot
added
the
[Status] Needs Review
There was a problem hiding this comment.
Copilot reviewed 1 out of 3 changed files in this pull request and generated no comments.
Files not reviewed (2)
- client/blocks/signup-form/style.scss: Language not supported
- client/login/wp-login/style.scss: Language not supported
Comments suppressed due to low confidence (1)
client/blocks/login/index.jsx:131
- Ensure that the behavior introduced by accessibleFocus() is covered by tests.
accessibleFocus();
There was a problem hiding this comment.
Copilot reviewed 2 out of 5 changed files in this pull request and generated no comments.
Files not reviewed (3)
- client/blocks/signup-form/style.scss: Language not supported
- client/layout/masterbar/oauth-client.scss: Language not supported
- client/login/wp-login/style.scss: Language not supported
adamwoodnz
force-pushed
the
fix/account-options-focus-styles
branch
2 times, most recently
from
f08e3ce to
7022718
Compare
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
adamwoodnz
force-pushed
the
fix/account-options-focus-styles
branch
from
13936e4 to
13d9e82
Compare
There was a problem hiding this comment.
Thank you, this is an outstanding step forward, and ready to land.
Note in the GIF above, that there are additional issues, I'm wondering if we should track this separately, it seems such a deep issue. It might even be worth discussing whether to remove the accessible-focus utility; if need be it can even be replaced with situational focus-visible, which seems a better way to do it.
The issues, blink and you'll miss it:
- The primary button does not have a focus style.
- All the "Continue with" buttons are Calypso components, whereas the email and jetpack buttons are Gutenberg components. The latter come with a subtle fading effect to the focus style, the former do not.
- Minor: the link does not have a focus style and falls back to the dotted browser default. It would be a better look with a single unified focus style across everything.
The real solution is to replace the Calypso components with Gutenberg components, though that's obviously a larger endeavour, and not in any way a blocker for this good step forward, or even urgent in their own right. It's just a slew of small papercuts that add up, and I'm mainly wondering your instincts on how to best track subsequent work here. Happy to open issues.
@jasmussen yeah I have made most of the same observations. My intention with this PR is to remove all the things which stop focus styles being displayed, so at least we meet accessibility standards. In #99915 I have started updating the existing focus styles for all the button variants (including primary) to match Gutenberg, but I hadn't considered actually replacing them with Gutenberg components. I think this sounds great (and feels on point with untangling) if we can do it incrementally, and I'll spike this approach with these screens to see how it shakes out. My concern is whether the existing accessible-focus styles would mess with the Gutenberg styles. If we've scoped both sets well it might not be an issue. |
Ok, I've reverted the accessible focus addition and replaced all the buttons with
|
|
Really nice work! Tested, this still works well. The focus styles were already inconsistent, so the fact that there's a single set of components that are now actually consistent, that's a good start.
Right, we need to carefully balance the effort here, I trust you to be mindful of your time and any competing priorities. This is papercut/product quality work, which is important, but nevertheless secondary to anything else that might be on your table. Because it's that category of work, it also makes sense to mainly do this right, just as you did here. The mix of component types here seems an oversight or rushed part of the puzzle, and the amount of deleted code in the update reflects that it's on a good path. Nice one! |
adamwoodnz
changed the title
adamwoodnz
force-pushed
the
fix/account-options-focus-styles
branch
from
1b18580 to
d99deb5
Compare
| border-bottom: 1px solid #c8d7e1; | ||
| color: var(--studio-gray-90); | ||
| display: block; | ||
| font-size: $font-body-small; | ||
| font-weight: 600; | ||
| padding: 0 24px; | ||
| text-align: center; | ||
| text-decoration: underline; | ||
|
|
||
| &:hover { | ||
| color: var(--color-primary); | ||
| } | ||
| &:last-of-type { | ||
| border-bottom: none; | ||
| } | ||
|
|
There was a problem hiding this comment.
I can't find an instance where there are 2 anchors in a magic login footer, so these border styles seem safe to remove.
adamwoodnz
removed
the
[Status] Needs Review
|
I've moved this back to in progress while I make the whole flow consistent |
adamwoodnz
force-pushed
the
fix/account-options-focus-styles
branch
from
5aa1099 to
8bd4c57
Compare
adamwoodnz
changed the title
| @@ -98,14 +99,14 @@ export default function ContinueAsUser( { | |||
| </button> | |||
| </div> | |||
| </div> | |||
| <Button | |||
| <A8CButton | |||
There was a problem hiding this comment.
I'm not familiar with (and don't know how to test) these other product screens so for now I've left them and focused on dotcom
| @@ -1,4 +1,5 @@ | |||
| import { Button } from '@automattic/components'; | |||
| import { Button as A8CButton } from '@automattic/components'; | |||
There was a problem hiding this comment.
This button has been marked as deprecated so I've chosen to make Button the WordPress components button, as that's what we should be using from now on.
| @@ -1089,13 +1094,14 @@ export class LoginForm extends Component { | |||
| { ! isBlazePro && <p className="login__form-terms">{ socialToS }</p> } | |||
| { shouldRenderForgotPasswordLink && this.renderLostPasswordLink() } | |||
| <div className="login__form-action"> | |||
| <FormsButton | |||
There was a problem hiding this comment.
FormsButton is basically an A8C submit Button with some default text, which won't be used here because children are being supplied.
| @@ -295,41 +267,6 @@ body.is-section-accept-invite { | |||
| display: flex; | |||
| flex-direction: column; | |||
| width: 100%; | |||
|
|
|||
| .social-buttons__button { | |||
There was a problem hiding this comment.
These styles are now centralized in client/components/social-buttons/style.scss
| @@ -140,7 +140,6 @@ const StepContainer: React.FC< Props > = ( { | |||
| cssClass={ clsx( 'step-container__navigation-link', 'has-underline', { | |||
| 'has-skip-heading': skipHeadingText, | |||
| } ) } | |||
| borderless | |||
There was a problem hiding this comment.
This is the default, so does not need to be passed
| @@ -158,7 +157,6 @@ const StepContainer: React.FC< Props > = ( { | |||
| handleClick={ goNext } | |||
| label={ nextLabelText || translate( 'Continue' ) } | |||
| cssClass="step-container__navigation-link" | |||
| borderless={ false } | |||
There was a problem hiding this comment.
This has primary: true so isn't necessary to be passed
| @@ -236,6 +236,7 @@ $breakpoint-mobile: 782px; //Mobile size. | |||
| .login__form-action { | |||
| button { | |||
| width: 100%; | |||
| height: 44px; | |||
There was a problem hiding this comment.
Login form inputs and submit buttons are taller across the board (44px vs 40px). This might be temporary if we decide to standardize them.
| padding-inline: 16px; | ||
|
|
||
| > svg { | ||
| margin-right: auto; | ||
| } | ||
|
|
||
| > span { | ||
| margin-left: -20px; | ||
| margin-right: auto; | ||
| } |
There was a problem hiding this comment.
Has the WordPress logo on the left, so needs to match the layout of the social buttons below
There was a problem hiding this comment.
Thank you @adamwoodnz! I tested the woo login and signup flows and can confirm that the focus styles are much better than before. 👍 I think using the Woo's purple color for the focus styles would be better, but we can do that later.
I've observed that the border and icon position of the social buttons have been altered. It would be preferable to retain the original design. Could you ensure that the original design is preserved? Thank you!
Before:
After:
Figma:
tp2u3Nk9arcpeNMZcylNG4-fi-271_8254
Fixes #99615
Proposed Changes
Currently the Login and Signup flows have no visible focus styles for most buttons. This PR implements consistent styles across all the buttons in these flows:
@wordpress/componentsButton components@wordpress/componentsto reduce duplication of rules like font-sizeNote that no changes have been made within the onboarding screens other than login and signup; only the nav buttons (Back, Next, Skip) have changed.
Screenshots
WordPress.com
Crowdsignal
Woo
Jetpack
Why are these changes being made?
Testing Instructions
Pre-merge Checklist