Update content area of login screens to use StepContainerV2 #102811
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 (~1060 bytes added 📈 [gzipped]) Common code that is always downloaded and parsed every time the app is loaded, no matter which route is used. Sections (~3096 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 (~256 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. |
tellthemachines
force-pushed
the
update/login-screens-content
branch
4 times, most recently
from
de6c963 to
3db8cc9
Compare
| @@ -360,23 +359,6 @@ export class LoginForm extends Component { | |||
| } | |||
| } | |||
|
|
|||
| renderLoginFromSignupNotice() { | |||
There was a problem hiding this comment.
Moving this notice outside of the form so we can render it just below the page heading.
| > | ||
| { this.renderI18nSuggestions() } | ||
|
|
||
| <DocumentHead |
There was a problem hiding this comment.
It feels super weird to have this DocumentHead inside the Main component but I'm guessing removing it would require a much larger refactor 😅
There was a problem hiding this comment.
Why exactly? The only dependency of it is the canonicalUrl, which is defined right above the component.
There was a problem hiding this comment.
Because JSX is supposed to emulate markup, and in HTML you'd never have the document head inside the main content. The code may work correctly, but it feels horribly unsemantic.
There was a problem hiding this comment.
Not trying to speak on behalf of Luis, but I feel the question may have been "why is it a large refactor?".
There was a problem hiding this comment.
Oh! I noticed we're using DocumentHead like this in many places, so if we decided to move it somewhere where it makes more sense semantically we should probably be consistent and do it for all instances.
There was a problem hiding this comment.
For what is worth, Next.js also has a Head component that does exactly the same thing: set something in the <head> element.
Doing it with the proper nesting and "semantically" is probably outright impossible, because React apps never control the entire HTML document, they are always mounted in some sub-element of <body>.
There was a problem hiding this comment.
React 19 does support all the metadata tags dropped anywhere in your component tree, so hopefully at some point we'll be able to migrate to it 🤞
| } | ||
| heading={ <Step.Heading text={ headerText } /> } | ||
| > | ||
| { mainContent } |
There was a problem hiding this comment.
We could further break down this mainContent to use StepContainer wrappers but that will require taking apart more bits of LoginBlock. It might be better left to a follow-up PR because this diff is already pretty big.
(I'm not 100% sure we need to have a LoginBlock component, because they're both looking at similar conditions to render things. There's no real separation of concerns.)
matticbot
added
[Status] Needs Review
tellthemachines
added
[Status] Needs Design Review
matticbot
added
the
[Status] Needs Review
github-actions
bot
added
the
[Status] Design Input Requested
|
Note there are a bunch of other screens using the same Login components that should not be affected by these changes. These are the ones I have unearthed so far: Gravatar 
Gravatar-powered WP Job Manager: 
Jetpack: 
Blaze Pro: 
Woo: 
(The last two are pretty similar to the |
|
I don’t know how to run the PR in my local, but when I clicked on the calypso live link, I was able to land on the login page ( The 6-cols container has a side padding that shrinks the area where actions are.
On the smallest breakpoints, the spacing breaks the alignment. 
When changing the viewport from CleanShot.2025-05-01.at.11.04.42.mp4The whole content area is not vertically aligned to balance the top and bottom spaces according to the browser window, leaving an empty space at the bottom.
I acknowledge the PR scope is not about copy change, yet I want to point out the different in
Regarding your questions
Pinging @crisbusquets and @nuriapenya in case they know about an strategy for dealing with language change call outs. I remember seeing some tickets mentioning a general revision, but I'm unaware of the outcome.
We're working on a new Notice component (dhW7zYBamXJIeJIE3wy5f4-fi-111_840), and on this thread (p1742928013644859/1742810438.892249-slack-CNGQYA3B9), there is an agreement to use an updated version of the Calypso component. But I'm unaware of the code state and the next steps. |
zaguiini
dismissed
their stale review
We're not overriding StepContainerV2 anymore!
|
Thanks for the reviews folks! I think I've addressed all the feedback so far.
This only happens on |
So if it's a rule for the whole form area, how is that at 482px the input and actions are full width? I don't want us to hack the max-width best practice, but I think that from |
There was a problem hiding this comment.
I'm noticing some things:
- I see the login form blink upon refresh (not happening in prod)
- It's not centered aligned on load. There's a layout shift
- I don't see the subheader text anymore. Is that expected? We have it in prod.
- Then I see a massive blank space between my user action and the change account escape hatch
- Finally, the page scrolls when it shouldn't
Registrazione.schermo.2025-05-02.alle.9.34.29.AM.mov
That layout shift happens even when entering the page as a guest:
Registrazione.schermo.2025-05-02.alle.9.38.01.AM.mov
Other than that, it's looking almost perfect. Some style comments:
- Depending on the screen size, there's some extra margin between the border and the UI elements. Can we make that consistent? By that, I mean always stretch it starting from the mobile breakpoint.
- The "Lost your password?" link has different styles compared to the "Create an account" one. It would be awesome if we could standardize that too.
Registrazione.schermo.2025-05-02.alle.10.17.27.AM.mov
Helpers
Login form blink
I managed to reproduce it both locally and on Calypso.live. This is caused by SSR, which as you'll read below, doesn't seem to be enabled in production? I don't know.
Layout shift
This is happening because the CenteredColumnLayout wireframes uses the context.isSmallViewport variable, which is false on SSR (after all, there is no viewport on the server), and we get the wireframe rendering its content at the top. This is very unfortunate 😅
When I enter production, I don't see the SSR part. There's no data-calypso-ssr="true" in div#wpcom. That's kind of weird. Maybe it's not using it in production? Or is it so fast I can't even notice it? 🤷🏻
Anyway... Luckily, the solution is straightforward (if we actually want to change it.) We'll need to delegate the decision of rendering the content at the center of the screen to the CSS realm instead of JS:
diff --git a/packages/onboarding/src/step-container-v2/components/ContentWrapper/style.scss b/packages/onboarding/src/step-container-v2/components/ContentWrapper/style.scss
index 5165e62bddf..52bfa8cf3f7 100644
--- a/packages/onboarding/src/step-container-v2/components/ContentWrapper/style.scss
+++ b/packages/onboarding/src/step-container-v2/components/ContentWrapper/style.scss
@@ -32,9 +32,11 @@
);
&.center-aligned {
- justify-content: center;
- padding-bottom: calc(
- var( --step-container-v2-content-block-padding ) + var( --step-container-v2-top-bar-height )
- );
+ @include break-small {
+ justify-content: center;
+ padding-bottom: calc(
+ var( --step-container-v2-content-block-padding ) + var( --step-container-v2-top-bar-height )
+ );
+ }
}
}
diff --git a/packages/onboarding/src/step-container-v2/wireframes/CenteredColumnLayout/CenteredColumnLayout.tsx b/packages/onboarding/src/step-container-v2/wireframes/CenteredColumnLayout/CenteredColumnLayout.tsx
index d4d29a6167a..e676dd7e1ab 100644
--- a/packages/onboarding/src/step-container-v2/wireframes/CenteredColumnLayout/CenteredColumnLayout.tsx
+++ b/packages/onboarding/src/step-container-v2/wireframes/CenteredColumnLayout/CenteredColumnLayout.tsx
@@ -33,7 +33,7 @@ export const CenteredColumnLayout = ( {
return (
<>
<TopBarRenderer topBar={ topBar } />
- <ContentWrapper centerAligned={ context.isSmallViewport && verticalAlign === 'center' }>
+ <ContentWrapper centerAligned={ verticalAlign === 'center' }>
{ heading && <ContentRow columns={ 6 }>{ heading }</ContentRow> }
<ContentRow columns={ columnWidth } className={ className }>
{ content }
Even with that, there's a small layout shift (restart the Calypso server for these changes to take effect since it's SSR), but I think it's caused by...
Page scroll fixer
The page scroll can be addressed by removing the padding-bottom and min-height from .layout.is-section-login.
Massive blank space in the "continue as user" block
Can be removed by setting height: auto and margin: 0 auto in .continue-as-user.
| > | ||
| { this.renderI18nSuggestions() } | ||
|
|
||
| <DocumentHead |
There was a problem hiding this comment.
Why exactly? The only dependency of it is the canonicalUrl, which is defined right above the component.
tellthemachines
force-pushed
the
update/login-screens-content
branch
from
6bef736 to
4c4fda0
Compare
|
Translation for this Pull Request has now been finished. |
There was a problem hiding this comment.
This is still testing well for me, and I like that the refactor splits out getHeaderText from the LoginHeader, and that the LoginHeader is extracted from Login. These feel like good steps forward to me 👍. getHeaderText's signature is pretty long and I could imagine it being easy to accidentally make a mistake on ordering of the params. Not a blocking comment, but if we were going to use this function in additional places, I'd suggest using an object as a param so that we don't need to worry about the positioning of the params.
I haven't been able to find any issues with each of the different logins I've attempted with this PR checked out, though it's very hard to be sure that I've covered everything, but from my perspective I think this change is looking pretty good to land. And we've certainly been covering the main known use cases a lot in our testing (WP.com, Woo, Jetpack, WPCloud, A4A, etc).
@jsnajdr I see you've been reviewing this PR — are you still looking through it, or do you think it's okay to ship? I'm aware it looks like there've been other discussions about ideas for how to improve the login code, so wasn't sure where this sits in priorities. IMO it'd be nice to get the design improvements from this PR in.
What we should do is to offer just a handful of focused customization points: color, logo, header text and font. The rest of the UI would be unified. That's something that's possible to maintain and test.
That does like a good idea for the long-term to me!
The intention is not to make it reusable 😅 as this header text is very specific to its context. I only extracted the function so I could keep that horrible pile of conditions unchanged while applying it both to the redesigned ( This PR has been going on for a while and I think it's in a pretty stable state now, so if no one is opposed I'm going to go ahead and merge it within the next few hours. Otherwise the chance of further merge conflicts is high and it's becoming a bit of a time sink. |
There was a problem hiding this comment.
Just re-tested, and I think there's one more area of styling that needs resolving. When I log in with an account that uses 2FA, the screen after submitting my username and passwords has alignment that's a bit off:
It looks similar to the margin: 0 issue we were looking at earlier?
Oh weird, I was sure I'd tested that. Thanks for spotting it! |
|
Ok I've fixed the margins on the 2fa screen, but I'm not really happy with the fix as I had to leverage the I opted for this because I don't want to further complicate and increase testing scope for this PR. I hope to follow up with another PR to untangle |
github-actions
bot
removed
[Status] String Freeze
@tellthemachines Oh, I apologize for being so unhelpful while attempting to be helpful 🙁
@andrewserong I got to review it again only now, after merge. Given that the login page is such a monster, it's really hard to determine if such a big change is OK to ship. I'm afraid we'll need to do a few post-merge fixes as problems are discovered. Here's the first one: the Woo Core Profiler Lost Password Flow (whatever that is 🙂) is broken. Go to: both logged in and logged out. The logged out page is broken in this way: There's a 
The logged-in version, with 
Additionally, the |
| ); | ||
| } | ||
| } | ||
| } else if ( isWooJPC ) { |
There was a problem hiding this comment.
When the flow is isWooJPC then isWhiteLogin is false and this LoginHeader component is not even rendered. That's one reason why this flow (including "lost password") has incorrect header and sub-header.
| const isLostPasswordFlow = currentQuery.lostpassword_flow === 'true'; | ||
| switch ( true ) { | ||
| case isLostPasswordFlow: | ||
| headerText = translate( "You've got mail" ); |
There was a problem hiding this comment.
There are missing break statements in this switch. The isLostPasswordFlow condition falls through to the following condtions and the headerText is overwritten.
Personally I would rewrite these switch ( true ) statements to a regular if ... else if ... else.
|
One more broken page, again the WooJPC flow. Double masterbar, and misaligned icons on the social login buttons: 
|
|
Thanks for reviewing and testing @jsnajdr! I suspect those Woo screens have been broken since #102353, when I updated Question: how did you get to those screens? If I go to
This was absolutely what I was expecting when I merged the PR! Given it had been open for 3 weeks and had a substantial amount of testing I was fairly sure nothing major was broken, but any large change to a piece of spaghetti like this is bound to break something. I'll follow up with some fixes 😄 |
I was reading the code, inspected the various conditions and then tried to reconstruct the actual URL to match that condition. I didn't test the full flows that start somewhere on woocommerce.com or inside a self-hosted site's WP Admin. With that approach, it's possible that some URLs don't really exist in practice. For example, it's possible that the real flow sends you to |
|
I'm not 100% sure it's related to this PR but I discovered an issue on the Blaze Pro 2FA screen where the logos are clashing in the header:
|
|
Good catch @adamwoodnz! @tellthemachines won't be around for the next few weeks, but I'm happy to try to help out here where I can. I've opened up #103537 with a tentative fix for the Blaze Pro 2FA screen, though I'm not 100% sure if it's the right way to go about it. As far as I can tell that screen's layout has been a bit broken for a while (possibly longer than this PR landing) That PR seems to be working okay for me so far, but might need a bit of extra testing to confirm. I can continue looking at that tomorrow! |
Related to #102039
Proposed Changes
Updates the login screens using
isWhiteLogindesign to make use of theStep.CenteredColumnLayoutwireframe, including rendering titles withStep.Heading.Removes Gravatar and GravPoweredClient login screens from
isWhiteLogincondition (they were already overriding most of the styles anyway)Re-adds WP Cloud logo to the WP Cloud login page which also uses
isWhiteLogin, and which I missed in Update wp.com login screen designs [WIP] #102353 😅In order to separate
isWhiteLoginscreens from all the other login screens using the same component, I extracted the header rendering logic into its own file, and further extracted the header text logic into its own function, so that we can use the header text forisWhiteLoginscreens, and still render all the extra pre- and post-header bits and pieces for all the other screens.Testing Instructions
Affects the following screens:
Content for "log in with existing account" should match the following design:
We're not quite there yet, mainly due to the notice used. Current state of this PR is:
There's also an additional notice that sometimes pops up about using different languages:
@fcoveram how should we deal with this other notice?
Also, should the existing account notice be using the WP component instead of the Calypso one?
Other affected screens
Pre-merge Checklist