-
Notifications
You must be signed in to change notification settings - Fork 2k
Prevent launchpad track events from firing when focused launchpad to be shown #102023
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
Conversation
- Introduced `useSite` and `getStepFromURL` hooks to manage site and step state. - Implemented conditional logic in `useTracksEventProps` across multiple flows to prevent tracking events from firing when the launchpad is active. - Ensured consistent handling of loading states for tracking events in the `build`, `newsletter`, `start-writing`, `update-design`, and `write` flows.
Link to live branch is being generated... |
Here is how your PR affects size of JS and CSS bundles shipped to the user's browser: App Entrypoints (~25828 bytes removed 📉 [gzipped])
Common code that is always downloaded and parsed every time the app is loaded, no matter which route is used. Sections (~1100 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 (~132951 bytes added 📈 [gzipped])
React components that are loaded lazily, when a certain part of UI is displayed for the first time. Moment.js Locales (~115 bytes removed 📉 [gzipped])
Locale data for moment.js. Unless you are upgrading the moment.js library, changes in these chunks are suspicious. 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 |
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 PR works as described, but I wonder what are the advantages of persuing this strategy instead of the previous one where we were preventing them to go to full screen launchpad at all.
That seems an approach more aligned with the end goal of removing the full screen launchpad entirely.
Should we fix the blog onboarding
bug and revert our revert as an ideal solution?
There are other areas that navigates to fullscreen launchpad separate from the |
If we go with the quick fix, then can you make an issue for the follow up work? With detailed info on the approach to take @vykes-mac ? Then we'll make that issue part of Launchpad Cleanup, which I assume is in our near future as part of work described in pet6gk-27S-p2 |
This PR commits have gotten messed up closing |
closed in favor of #102311 |
Closes #101706
Proposed Changes
This PR fixes the issue where events are being recorded for
fullscreen launchpad
even though we are not showing that launchpad. This happens because we fist navigate to theLaunchpad
step before deciding if we should navigate to/home
to show the focused launchpad. We have to update multiple flows as launchpad is not apart of the onboarding flow however the onboarding flow hand off to other flows after site setup is completed.useTracksEventProps
to prevent events from firing whenshouldShowLaunchpadFirst
and we are on the launchpad step. This works as we are immediately redirected to home and so events are not registered.Why are these changes being made?
Testing Instructions
/setup/onboarding
and go through the flowimg
and filter bycalypso_signup_step_start
calypso_signup_step_start
withstep=launchpad
should not be fired.Non onboarding test
/start/free
and go through the flowimg
and filter bycalypso_signup_step_start
calypso_signup_step_start
withstep=launchpad
is firedNetwork tab

Pre-merge Checklist