Update login form fields to match core components #103616
Conversation
Jetpack Cloud live (direct link)
Automattic for Agencies live (direct link)
|
|
Here is how your PR affects size of JS and CSS bundles shipped to the user's browser: App Entrypoints (~684 bytes added 📈 [gzipped]) Common code that is always downloaded and parsed every time the app is loaded, no matter which route is used. Sections (~21544 bytes added 📈 [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 (~152 bytes added 📈 [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. |
adamwoodnz
force-pushed
the
dsgcom-95-update-login-input-styles
branch
from
4c966a3 to
b124a43
Compare
| box-sizing: border-box; | ||
| width: 100%; | ||
| height: $grid-unit-50; | ||
| border-color: var(--color-neutral-10); | ||
| font-size: $font-body-small; | ||
| color: var(--color-neutral-60); | ||
|
|
||
| @include break-small { | ||
| font-size: $font-body-small; | ||
| } |
There was a problem hiding this comment.
All based on the design: YIwc478nGHyjIZ45hQ1hfV-fi-245_3240
| font-size: $font-body-small; | ||
| } | ||
|
|
||
| @extend %form-field-disabled; |
There was a problem hiding this comment.
Core styles don't seem to include disabled state, so extending the existing
There was a problem hiding this comment.
I'm not sure about the exact list of Core components you're referring to, but some components should already ship with disabled styles (for example, InputControl)
There was a problem hiding this comment.
Ah I see now; styles are added to the control container which we don't have here using the existing form controls. I'll see if I can also use a mixin for that.
adamwoodnz
force-pushed
the
dsgcom-95-update-login-input-styles
branch
from
5616e95 to
85d55e7
Compare
adamwoodnz
changed the base branch from
trunk
to
dsgcom-92-core-button-centralize-overrides
adamwoodnz
force-pushed
the
dsgcom-95-update-login-input-styles
branch
4 times, most recently
from
d7d77e7 to
8765545
Compare
adamwoodnz
changed the base branch from
dsgcom-92-core-button-centralize-overrides
to
trunk
adamwoodnz
force-pushed
the
dsgcom-95-update-login-input-styles
branch
from
8765545 to
5388620
Compare
There was a problem hiding this comment.
Follows the approach of centralizing overrides from #103611
matticbot
added
the
[Status] Needs Review
adamwoodnz
force-pushed
the
dsgcom-95-update-login-input-styles
branch
from
4db8bbb to
016de2d
Compare
|
This PR modifies the release build for the following Calypso Apps: For info about this notification, see here: PCYsg-OT6-p2
To test WordPress.com changes, run |
| file: args.in, | ||
| importer, | ||
| outputStyle: 'compressed', | ||
| includePaths: [ 'node_modules' ], |
There was a problem hiding this comment.
Team City build for reader mobile stylesheet was failing without this, as it imports _extend-forms.scss
|
Very nice. I appreciate continuing with the singular "style overrides" SCSS file as a singular place to store these. I envision these also eventually being able to update core PHP component styles. Should we update the label to the unified all-caps style that the core components have? One thing also, both help text for inputs, but also the validation pattern here, should have 🙏 Thanks for keeping on this! |
|
Added note related to labels, as part of content guidelines, we should move from titlecase to sentence case wherever we encounter it. Not a blocker at all, but noting, as it would affect at least this one:
These should be sentence case even if we use CSS to all-caps it. Painting the back of the fence here :D |
|
Also curious, @michaelpick, if it should be "Privacy policy" and "Terms of service"? Even as part of a sentence, should it then be "By continuing you agree to our [link]terms of service[/link] ..." ? |
Not sure about this, to my eye it looks shouty, especially with other brand styles, eg. A4A Screenshots
The design (YIwc478nGHyjIZ45hQ1hfV-fi-245_1996) shows sentence case, what do you think? |
adamwoodnz
force-pushed
the
dsgcom-95-update-login-input-styles
branch
from
859e154 to
6c49299
Compare
I don't see it that much as shouty, one of the benefits especially when the labels are shorter is that it becomes more of a bar, optically. Given an incoming unification of these screens that style should still be part of it: p58i-lKQ-p2 That said, thanks for surfacing the inconsistency added a comment in the Figma. The goal with this work is in part getting the details right, but more importantly it's about reducing the unintentional divergence and maintenance that comes with that. Any exception, thus, has a small price tag worth being sure about! |
There was a problem hiding this comment.
What's the relationship between this file and client/assets/stylesheets/_wp-components-overrides.scss ?
I see that we're applying some overrides to all input elements, and I wonder if those overrides are also expected to "bleed" into components from the @wordpress/components package.
There was a problem hiding this comment.
The overrides in this file at present do not affect @wordpress/components, and only override --wp-* vars set by the base styles package.
I see that we're applying some overrides to all input elements, and I wonder if those overrides are also expected to "bleed" into components from the @wordpress/components package.
They're intentionally not overriding all input elements or bleeding into @wordpress/components at present; only those with the opt in class .form-text-input-core-styles are affected. For the core input components I imagine we'll want to add separate overrides to _wp-components-overrides.scss, but they're not in use in this screen due to the SSR incompatibility.
I think some documentation in this file would help, and perhaps it should be restructured so that the outer selector is .form-text-input-core-styles
/* Extends @wordpress/base-styles/mixins input-control() */
.form-text-input-core-styles {
.blaze-pro &,
.is-blaze-pro & {
&:not(.is-error) {
--wp-admin-theme-color: var(--color-gray);
}
}
.woo.is-woo-passwordless & {
...
}
}
There was a problem hiding this comment.
| &.form-text-input-core-styles { | ||
| /* stylelint-disable-next-line scales/radii */ | ||
| border-radius: 8px; | ||
| font-size: 1rem; | ||
| height: 48px; | ||
| padding: 8px 16px; | ||
|
|
||
| &:not(:focus, :disabled) { | ||
| border-color: var(--studio-gray-30); | ||
| } | ||
|
|
||
| &:focus { | ||
| border-width: 1.5px; | ||
| } | ||
|
|
||
| &:not(.is-error) { | ||
| --wp-admin-theme-color: var(--woo-purple-50); | ||
| } |
There was a problem hiding this comment.
Overriding aspects beyond colors, such as border radius, font-size, height, and padding makes me a bit uncomfortable — why are those overrides needed? Can we just use existing components instead, with a light "theming" touch through color overrides?
There was a problem hiding this comment.
Woo has heavy customisations of both inputs and buttons:
| Default | Focused |
|---|---|
 |
 |
I agree it's quite a maintenance overhead, but I'm not sure if this is the PR to change that, as the scope will be large as it is. I think it's helpful to at least have the overrides centralized here, and we can look to reduce them in consultation with Woo design?
| input[type='password'] { | ||
| &.form-text-input-core-styles { | ||
| &:not(:focus, :disabled) { | ||
| border-color: #9ea4a8; |
There was a problem hiding this comment.
If there's no variable for this color, maybe we can add an inline comment explaining if there's at least a reference to is in the design specs?
There was a problem hiding this comment.
I tried to maintain the original, but I think there's a strong case here for using the default.
There was a problem hiding this comment.
Removed in 4cae5aa
client/assets/stylesheets/style.scss
Outdated
|
|
||
| // Overrides for @wordpress/base-styles | ||
| @import "./wp-base-styles-overrides"; |
There was a problem hiding this comment.
Should the base style overrides be applied before the component overrides?
There was a problem hiding this comment.
Maybe? I haven't seen any adverse affects, but that's probably because the base overrides are all very targeted and don't affect the components at this point. I'll swap them in case that changes in future.
| @@ -430,7 +436,7 @@ export class LoginForm extends Component { | |||
|
|
|||
| <FormLabel htmlFor="usernameOrEmail"> | |||
| { this.isPasswordView() ? ( | |||
| <Button | |||
| <A8CButton | |||
There was a problem hiding this comment.
Given that Button from @automattic/components is deprecated, can this be an opportunity to switch to the Button component from @wordpress/components ?
There was a problem hiding this comment.
This is a different Woo form to the one I've seen in production, and as such I haven't touched it at all. I've intentionally tried to keep to a limited scope but I'm aware this has likely created inconsistencies.
- I'll find out how to test this flow.
There was a problem hiding this comment.
Not specific feedback for this PR, but I'm generally uncomfortable with the amount of style overrides that we apply to form components.
I'd love it if we could move to re-using existing shared componentry with a minimum amount of style overrides (mostly for theming purposes)
There was a problem hiding this comment.
I've added more screenshots in the description to demonstrate all the variants we currently have for the brands.
Quoting this reply that I left earlier on p1748419987948939/1748419029.140069-slack-C05QAFZSFTL :
|
If we choose not to create a variation and leave the labels small allcaps, I think the 2 variations that really need attention are A4A and Jetpack Cloud which have a larger font size and lighter weight applied:
I think we should remove these customisations. |
| /*!rtl:ignore*/ | ||
| padding-right: 32px; | ||
| padding-right: 36px; |
There was a problem hiding this comment.
Allows more room for password manager UI
I would agree with that, and this is being discussed also in context of p58i-lKQ-p2. For awareness, @keoshi, though. |
|
Thanks for the ping, @jasmussen! Removing customizations sounds good to me. However, it seems we're going with allcaps and I'd like to understand more why. I seem to remember a recent conversation and an argument from @fcoveram about how these screen follow Dotcom's styling rather than Core. Has anything changed since then? |
|
There are a couple of different threads ongoing here, where Adam has been fixing some claring accessibility issues in the login forms, those have been based on previous direction based on default componentry. In that case, all caps labels. However I'm also in the conversation where the general consensus is that the all-caps labels don't generally make sense for every instance, and that it will be useful for this componentry to have exceptions to this. In that sense, what I am agreeing with is: 1) unifying the codebases, 2) going with the same overall text style for labels. If those two happen at the same time, that works for me too. |
| aria-hidden={ isHidden } | ||
| tabIndex={ isHidden ? -1 : undefined } |
There was a problem hiding this comment.
Ensures the button is not accessible when this field is in the DOM but hidden visually
Reduce label selector scope to not match 'Show password', and thereby return multiple items
Increase button count as the password toggle is now a button Select the submit button by text as there are multiple buttons
| @@ -17,7 +17,7 @@ describe( 'LoginForm', () => { | |||
| const username = screen.getByLabelText( /username/i ); | |||
| expect( username ).toBeInTheDocument(); | |||
|
|
|||
| const password = screen.getByLabelText( /password/i ); | |||
| const password = screen.getByLabelText( /^password$/i ); | |||
There was a problem hiding this comment.
Restricts the match to only 'Password' and not 'Show password'
Done in 208eb43 If we're not going with small allcaps for these we'll need a variation as mentioned above. Maybe setting a prop on the label component |
Part of DSGCOM-95: Update text fields on login
Alternative to #103319, using core styles only and not replacing the field components
Proposed Changes
FormTextInputandFormLabelwhich if passed styles them using@wordpress/base-stylesKapture.2025-05-29.at.16.58.18.mp4
Why are these changes being made?
Testing
Screenshots
Instructions
Login links
WordPress.com
Crowdsignal
Jetpack Cloud
Woo
Blaze Pro
Akismet
A4A - Can't link due to GitHub OSS scan rules: go to automatt_c.com/for-agencies/ and click 'Log in'. Edit the URL and change the host to calypso.localhost:3000
Gravatar
WP Job Manager
VIP
Partner Portal - Can't link due to GitHub OSS scan rules: go to hosts.automatt_c.com and you should be redirected to log in. Edit the URL and change the host to calypso.localhost:3000
WP.Cloud - Can't link due to GitHub OSS scan rules: go to wp.cloud and click 'start here'. Edit the URL and change the host to calypso.localhost:3000
Pre-merge Checklist