-
Notifications
You must be signed in to change notification settings - Fork 2k
Fix Woo login layout issues #103445
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
Fix Woo login layout issues #103445
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 (~12 bytes removed 📉 [gzipped]) DetailsCommon code that is always downloaded and parsed every time the app is loaded, no matter which route is used. Sections (~33 bytes removed 📉 [gzipped]) DetailsSections contain code specific for a given set of routes. Is downloaded and parsed only when a particular route is navigated to. Async-loaded Components (~33 bytes removed 📉 [gzipped]) DetailsReact 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. |
Hey @tellthemachines, thanks for the ping! The flow is triggered when a user complete onboarding wizard in a WooCommerce store. Steps for the flow:
This is full URL that it would be like:
I think it's unrelated to your PRs, but the button colors are incorrect. They should be wp theme color (blue), depending on the store's primary color.
Showing the progress bar is expected. |
andrewserong
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! I did a quick check of WordPress.com, Blaze, Jetpack, and A4A logins, and they were all looking good.
I found one more login while playing around with WooCommerce after installing on a Jurassic Ninja site. It appears when you click to on "Connect Jetpack" from the Stats card after finishing the WooCommerce onboarding (this is the button):
It looks like it's a styled version of the Jetpack login? A simplified test URL that appears to get to this login state is this one: https://wordpress.com/log-in/jetpack?from=woocommerce-onboarding
It displays the login form and social icons stacked, with the OR text in the wrong position:
That could be looked at separately to this PR, though.
I think it's unrelated to your PRs, but the button colors are incorrect. They should be wp theme color (blue), depending on the store's primary color.
Similarly, this feels like a lower priority issue to me (but thanks for flagging it!)
I reckon this PR will be good to get in 🚀
|
Thanks for the detailed instructions @chihsuan! I don't think those are the exact same screens though. Setting up a JN site and going through the WooCommerce onboarding (after setup I was taken to wp-admin so had to click on "WooCommerce" in the sidebar) this is what I'm getting: "Create an account" looks like it belongs to the same family design-wise, but it doesn't appear broken in production like the screens @jsnajdr found: This is login: The lost password screen I'm looking for should say "You've got mail" in the heading. It would be great to get to the bottom of this, because if by any chance those screens aren't in use any more, there's a bunch of logic we can remove from the login components 😄 |
|
Update: just realised I borked the lost password heading in extracting |
|
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 |
To clarify, the
I'm not sure what's going on, but the screens look completely broken. 😢 The login screen should be look like this: And create an account screen:
It should display "Forgot your password?" in the heading.
And after users enter their email address and click on the reset button, it should be changed to "You've got mail". |
Looks like the button colors changed in #102639, because these screens also have the |
|
Ok the bad layout seems to mostly be due to a wrapper added around the form contents so they no longer get the flex rules applied to the
Those changes were introduced in #102877, which updated the Jetpack login so they were probably not meant to also affect this screen. |
Thanks for looking into it! 🙏 @tellthemachines FYI, the team in Woo who maintain these screens is gone, I'm also not working on this anymore. However, please feel free to let me know if anything requires review or testing. I have also raised the issue with my team lead to see if we can get some help on this. |
| } | ||
|
|
||
| if ( isFromMigrationPlugin ) { | ||
| } else if ( isFromMigrationPlugin ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm curious about this "migration plugin" flow. It's active when there is a from=wpcom-migration query param, and its layout is now also broken.
@jeherve @anomiex Is this wpcom-migration plugin and the redirects to /log-in?from=wpcom-migration (or Jetpack Connect with the same query param) still active? I see that the Automattic/wpcom-migration repo has been archived a few months ago, the "Move to WordPress.com" plugin is no longer in the .org repo, and there is only a "Migrate to WordPress.com" plugin based on BlogVault.
That sounds almost like this flow is no longer used and could be removed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The version of the plugin that connected to WordPress.com via this flow is indeed no longer in use. The new version of the plugin doesn't use that flow. I think we could remove it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All I know about the wpcom-migration is that it was created in the monorepo, had a few releases, then nothing for a while until apparently they decided to replace it with a reskin of an existing (external) plugin. Whether this code path is still used from somewhere (e.g. the existing plugin from the external partner) I have no idea.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds like a good candidate for a follow-up PR!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Following up in #103766 which removes most of the wpcom-migration customizations.
|
Thanks for the ping @tellthemachines ! I'll review this from our end and see if I can spot anything. I'll also double-check if any flow is deprecated so we can remove it. |
|
@elazzabi thanks, that would be very helpful! I tried testing the lost password screen again based on what @chihsuan mentioned above:
After entering email and clicking reset this is what I'm seeing:
The URL for this screen is So it's not exactly the same screen we get with Are we still using any non-Jetpack login for Woocommerce? If not, we should be able to remove some of the conditionals from the Login components. |
|
Thanks everyone for the feedback and suggestions on this PR! Whether those particular screens are in use or not, this still fixes logic that is causing some lost password screens to get the wrong heading, so would be good to merge soon. If Woo folks are happy with the changes, feel free to go ahead and merge it, otherwise I'll leave it until Monday. It's already Friday afternoon here, which is an inauspicious time for deployments 😅 |
Sounds great. +1 to merging this on Monday if it's still open by then. Thanks for the follow-up work here! |
We're using non-Jetpack login for WooCommerce.com link but not for core profiler/onboarding flows that coming from WooCommerce plugins. We don't need to fix anything for the |
client/blocks/login/login-header.tsx
Outdated
| } | ||
| ); | ||
| if ( isLostPasswordFlow ) { | ||
| header = <h3>{ headerText }</h3>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here the header assignment can be extracted out of the condition, it's always the same. Only the subtitle is conditional.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done!
elazzabi
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
|
The "lost password" flow still has a broken
This is probably not a realistic scenario: going through the lost password flow while being logged in. But the reason for the breakage is interesting: it's the |
That breakage wasn't introduced in that PR though. Blame shows the styles causing breakage have been around for over a year, probably from when the current passwordless screen was designed. In any case, the double header is now fixed:
Yes, and I believe there's some ongoing design work for unifying all these screens across brands. Even before that there's probably some cleanup that can be done on those styles while preserving the current designs. It's better addressed as a follow-up PR though.
The single-column layout with the 'OR' misaligned was introduced in #102877 and should be fixed separately as it'll require making sure both the Woo and the Jetpack logins are unaffected. Cc @gmjuhasz If all else is good, I'll go ahead and deploy this in the next couple of hours. |













Follow-up from #102811 and #102353.
As per @jsnajdr's comment, a few of the Woo login screens look a bit broken after my recent changes.
This PR fixes those issues by making sure WooJPC screens don't accidentally get treated as
isWhiteLogin. It also fixes the faulty switch statement that was causing some incorrect headings.The screens in question can be directly reached with the following URLs:
http://calypso.localhost:3000/log-in?from=woocommerce-onboarding&lostpassword_flow=true
http://calypso.localhost:3000/log-in?from=woocommerce-core-profiler
Because I don't know how a regular user would reach these URLs (they're different from both login and get started > wordpress.com flows available from https://woocommerce.com/), I'm not sure if they would ever display exactly in that shape. Given one of the conditions for
isWhiteLoginto be true is the query not containing aclient_id, if the path to those screens were to inject that query parameter they would display differently.Testing Instructions
(The weird blue line at the top of the second one is a
masterbar__progress-barand I'm not sure why it displays there but it seems unrelated to my PRs)Pre-merge Checklist