-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Stepper: Fix navigating to a gated step #98083
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 (~183 bytes added 📈 [gzipped])
Common code that is always downloaded and parsed every time the app is loaded, no matter which route is used. 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. |
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 |
@arthur791004 hi! I would really appreciate a review considering you and I are the most familiar with this part. |
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.
Tested all requested scenarios - everything works as expected.
I don't see any extraneous calypso_signup_step_start
being fired.
I also tested pointing the browser directly to /setup/onboarding/user
while logged out: fires calypso_signup_step_start {step: domains, skip_step_render: true}
, also as expected. Note in this case the goals
and design-setup
steps are never invoked, after user step we land in domains
and proceed from there.
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.
This test well for the described scenarios. It solves a lot of event duplication concerns that are mentioned here. Can we get this merged soon.
Oops...I'm in GK this week and didn't notice the ping 🙈 |
No worries! |
Fixes #98059
Proposed Changes
Stepper currently allows flows to navigate to a gated step while logged out, then it detects your auth status and if you're not logged in, it will redirect you to login. This is a roundabout way to redirect to auth when we have all the information we need before going to the gated step.
This change redirects the user to login before redirecting them to a gated step, without navigating to the gated step at all.
Why are these changes being made?
For a saner tracks narrative.
Testing Instructions
This needs a bit of thorough testing.
In-Stepper auth
calypso_signup_step_start
.calypso_signup_step_start
.calypso_signup_step_start
.calypso_signup_step_start
.calypso_signup_step_start
.calypso_signup_step_start
.calypso_signup_step_start
.calypso_signup_step_start
.calypso_signup_step_start
.Legacy auth
This tests auth that uses legacy non-Stepper auth
calypso_signup_step_start
.calypso_signup_step_start
.calypso_signup_step_start
.newsletterSetup
page and see one morecalypso_signup_step_start
.