-
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: Make framework tracking of submit-step the default. Updates remaining flows #93319
Stepper: Make framework tracking of submit-step the default. Updates remaining flows #93319
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 (~44 bytes removed 📉 [gzipped])
Common code that is always downloaded and parsed every time the app is loaded, no matter which route is used. Sections (~1069 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 (~51 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. |
3ddc3b6
to
6c3ebdb
Compare
d6a363a
to
99aea8b
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 |
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.
BEAUTIFUL! Wow man, removed so much noise!! Thank a lot. Found one tiiiny issue.
const siteSlug = useSiteSlug(); | ||
const flowToReturnTo = useQuery().get( 'flowToReturnTo' ) || 'free'; | ||
|
||
// eslint-disable-next-line @typescript-eslint/no-unused-vars |
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.
Do we need this?
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.
There are alternatives. But what do you suggest?
These sort of "fixes" are usually a sign of the code that underlies it. In this case, you really didn't need the two arguments signature with a spread on the second. A single object would have been better
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.
My bad, I didn't know providedDependencies
was not used at all underneath. I thought it was a mistake.
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.
Testing now..
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 3 random flows and they work exactly like intended. Tested flows that need intent
and ones that don't. Great work.
Thanks @alshakero 👍 Can we also take a look at #93273 and #93275 so we can wrap up this part? |
6c3ebdb
to
7c4cb04
Compare
99aea8b
to
faddf53
Compare
7c4cb04
to
f5ee38d
Compare
faddf53
to
b80a380
Compare
f5ee38d
to
f65d38f
Compare
b80a380
to
3f30f0c
Compare
f65d38f
to
e4e378b
Compare
b5dcd29
to
b1f6ea3
Compare
e4e378b
to
86d181d
Compare
b1f6ea3
to
654df6e
Compare
Related to #91755
Proposed Changes
recordSubmitStep
recordSubmitStep
to pass it through the data-store:readymade-template
,link-in-bio-post-setup
,newsletter-post-setup
,update-design
,update-options
intent
is provided and logged for all of these flows. Only for those that already recorded a value for it in their localised calls torecordSubmitStep
The PR touches several flows. We've tested individual migrations in previous PRs, so this follows along.
Why are these changes being made?
Part of addressing #91755
Testing Instructions
recordSubmitStep
is (at least currently) only called fromuseStepNavigationWithTracking
calypso_signup_actions_submit_step
is logged with correct values (in Netowrk tab, check for.gif
request) when navigating from/to any stepintent
is logged correctly as the name of the flow:/setup/readymade-template
/setup/link-in-bio-post-setup
/setup/newsletter-post-setup
/setup/update-design
/setup/update-options
Pre-merge Checklist