-
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
Import flows: Refactoring: useQuery instead of redux; update CTA text... #85784
Conversation
Here is how your PR affects size of JS and CSS bundles shipped to the user's browser: App Entrypoints (~45 bytes removed 📉 [gzipped])
Common code that is always downloaded and parsed every time the app is loaded, no matter which route is used. Sections (~2877 bytes added 📈 [gzipped])
Sections contain code specific for a given set of routes. Is downloaded and parsed only when a particular route is navigated to. 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. |
ab79e07
to
f30e02d
Compare
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.
Test various flows and all works as expected, great job!
Now, the import-ready preview screen is not redirected back to the capture screen if you press refresh. It will stay on the same page;
I'm unsure how to test this though, for me the import-ready preview screen seems not existing in the flow anymore (Same behavior on production). Only see it flickering between the steps and land on the next steps.
Screen.Recording.2023-12-29.at.3.57.49.PM.mov
We have recently removed that screen from the WordPress flow, and if you want to test it. You can try different source platforms, for example: "medium," "wix," or "Squarespace." |
Thanks for the explanation! Tried to test the wix following the instructions but when I refresh and click back button it still goes back to the capture screen. Am I missing something here? Screen.Recording.2024-01-02.at.11.07.08.AM.mov |
Removed dependency of the capture step, which, before this change, provided the urlData. Now, the fetching urlData logic is there.
This allows us to get cached analyzed data for the same URL between steps.
We need this to keep only siteSlug queryParam for the 'import' step.
This allows us to have direct access support of ready screens, getting rid of urlData dependency from the capture screen.
f30e02d
to
e7d612a
Compare
From my point of view, this is expected behavior. |
Ok, in this case, I have no other questions here 👍 |
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 |
…... (#85784) * Rename obsolete "Start building" to "Back to goals" button text * Update generateStepPath helper method supporting goals step * Ready steps: Update logic to allow direct step access Removed dependency of the capture step, which, before this change, provided the urlData. Now, the fetching urlData logic is there. * Ready component: Update effect dependencies * useAnalyzeUrlQuery: Update stale time to 5 seconds This allows us to get cached analyzed data for the same URL between steps. * site-setup flow: Match goToStep logic with import-focused-flow We need this to keep only siteSlug queryParam for the 'import' step. * Capture step: Update logic proving fromUrl as query param This allows us to have direct access support of ready screens, getting rid of urlData dependency from the capture screen. * Capture screen: Get rid of redux connect data; simplify component
Related to #85711
Proposed Changes
Testing Instructions
/setup/import-focused?siteSlug={SLUG}
/setup/site-setup?siteSlug={SLUG}
/setup/import-hosted-site
Note: For easier review, follow commit list (e2e tests are green 👌)
Now, the import-ready preview screen is not redirected back to the capture screen if you press refresh. It will stay on the same page; the "from" query param is included.
This is the required changes for future moves related to the task: #85711
Pre-merge Checklist