-
Notifications
You must be signed in to change notification settings - Fork 2k
Site Setup: Use flow entry point to manage back button edgy cases #102224
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
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: Sections (~143 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. |
@@ -746,6 +759,14 @@ const siteSetupFlow: FlowV1 = { | |||
selectedGlobalStyles, | |||
skippedCheckout, | |||
] ); | |||
|
|||
const { get, set } = useFlowState(); |
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 change is part of the useSideEffect() hook (Github way is not making it clear)
client/landing/stepper/declarative-flow/flows/site-setup-flow/site-setup-flow.ts
Outdated
Show resolved
Hide resolved
I don't see the Migrate vs Import step when I go through the described testing instructions with this branch. Is that intentional? Screen.Recording.2025-03-31.at.11.03.53.AM.movMinor note for the testing instructions, don't forget to add the |
Fixed 3540f96 Sorry about that, I forget to push 🤦
Thanks, I updated the docs |
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 |
I think we're losing the Screen.Recording.2025-03-31.at.11.44.57.AM.movIt behaves correctly when I manually re-add the |
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.
LGTM!
I'm really not a big fan of this 😢 It's tying two+ flows in untraceable ways. It would be great if you can explore removing My take would be
This will outsource the task to the browser and align the back button of the flow with the one from the browser. Note that I haven't tried this, but I suspect it may work with much less complexity. |
I hated this solution 😠 and I don't think it is the final solution. The big issue regarding the I think the ideal solution is site migration and site setup + adopting the new callback @daledupreez is suggesting here so one flow can point directly to another using the window. The document.referrer also doesn't work because it only returns the origin hostname, and we have multiple points where this flow can be triggered. Docs about https://developer.chrome.com/blog/referrer-policy-new-chrome-default |
Merging to unblock the code blue issue |
Related to:
Warning
This current solution doesn't ensure an alignment between our UI back button and the browser back button. Make them aligned is part of a refactor initiated on #101550.
Proposed Changes
migration
>entryPoint
toflow
>entryPoint
because now it will be used outside the migration flow context.The overall idea is the PR will introduce a specialized
ref=wp-admin-importers-list
, which will be stored on the newsite-setup
flow state to be reused when the user advance in onsite-setup
steps and use the back button.We must also repass this ref query param to the site migration flow to ensure the user can properly return to the /import.php page.
Why are these changes being made?
This PR ensures the user can return to the original page when they start the flow from there, regardless of whether they continue on the site setup or continue the site migration flow.
Testing Instructions
/setup/site-setup/importList?siteSlug={YOUR_SITE_SLUG}&ref=wp-admin-importers-list
Pre-merge Checklist