-
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
/start/onboarding always redirects to /setup/onboarding #97958
/start/onboarding always redirects to /setup/onboarding #97958
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 (~61 bytes removed 📉 [gzipped])
Common code that is always downloaded and parsed every time the app is loaded, no matter which route is used. Sections (~378 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 (~286 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. |
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.
When I enter the flow from /sites > Add new site, it redirects to /start -> /setup/onboarding -> Home of primary site (I think)
demo.mp4
Directly going to /start does correctly redirect to /setup/onboarding
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 |
Tested various entry points and they all worked as expected. I wonder if, since we're switching to use Stepper, shouldn't we change LOHP to directly point to /setup/onboarding, so to avoid a redirect. |
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 think we should redirect on the server to avoid loading all /start and only then redirecting to Stepper. These are precious cycles before the LCP.
Don't be blocked by my comment, though because this PR does the job. You can redirect on the server like this if you'd like to address this later.
'calypso_signup_onboarding_stepper_flow_confidence_check_3' | ||
); | ||
|
||
if ( isOnboardingFlow( flowName ) ) { |
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 think a lot of the logic above this will run for no reason. Maybe hoist this up as high as possible?
export const getHasRedirectedForExperiment = () => | ||
ignoreFatalsForStorage( () => | ||
sessionStorage?.getItem( | ||
'wpcom_redirected_calypso_signup_onboarding_stepper_flow_confidence_check_2' | ||
) | ||
); | ||
|
||
export const setHasRedirectedForExperiment = () => | ||
ignoreFatalsForStorage( () => | ||
sessionStorage?.setItem( | ||
'wpcom_redirected_calypso_signup_onboarding_stepper_flow_confidence_check_2', | ||
'1' | ||
) | ||
); | ||
|
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 agree 100%, but we should still redirect because the URL /start is long-established and is linked to from many places. |
Totally right @alshakero, I should have said that I suggested to do that and also the redirect ✌️ |
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.
It works as expected, nice work!
I think we should redirect on the server to avoid loading all /start
Agree that we can additionally perform this redirect by changing the LOHP. Do we plan to do that?
Yes. But does not have to be done for today's launch. But feel free to do asap. |
I thought we were going to have to go through all the LOHP's before we could get any performance improvements, but I hadn't thought about redirecting on the server, that's a good idea @alshakero. I'm going to merge as is so we can inch closer to launching the experiment. Luckily (for our experiment at least) both control and treatment will be subject to the same redirecting. |
Related to #96659
This is a pre-requisite for #97699
Proposed Changes
Now the stepper confidence check is complete p8AJCL-Zq-p2 we can take all onboarding flow traffic for
/start
and redirect it to/setup
.calypso_signup_onboarding_stepper_flow_confidence_check_3
logic and assume all users would have been in thestepper
variant.?redirect_1220
to prevent double redirectioncalypso_signup_onboarding_aa_test
, which is also now completedAs far as I can tell
?redirect_1220
was there to stop redirects happening when the legacy signup framework redirected back to the signup framework again. Now we're always redirect to the stepper this is no longer needed. It also caused issues where navigating to/start
would only redirect me to the stepper on the first time (because of the session storage). But with this PR we always want/start
to redirect to stepper. So it was getting in the way of that.Why are these changes being made?
The control for the Goals-First test should be Stepper, not Classic/Start. So before releasing the goals-first test we need all (onboarding flow) users to be using the stepper. Goals-first test: pbxNRc-4GM-p2
Testing Instructions
/start
, you should be taken to the stepper framework./start
again, you should once again be taken to the stepper frameworkPre-merge Checklist