Stepper Example Flow: Integrate site creation and state management #99676
Conversation
…ypso into fix/preload-after-user
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 (~255 bytes added 📈 [gzipped]) Common code that is always downloaded and parsed every time the app is loaded, no matter which route is used. Sections (~3407 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. Async-loaded Components (~12 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. |
…site title retrieval
matticbot
added
the
[Status] Needs Review
|
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 |
| await saveSiteSettings( providedDependencies?.siteSlug, { | ||
| launchpad_screen: 'full', | ||
| } ); |
There was a problem hiding this comment.
We should bake this into the pending action above. Something like:
createSite( {
theme: DEFAULT_NEWSLETTER_THEME,
siteIntent: Onboard.SiteIntent.Newsletter,
} ).then( siteCreationResult => {
// update site settings but return the siteCreationResult when done.
return saveSiteSettings( siteCreationResult.siteSlug, {
launchpad_screen: 'full',
} ).then(() => siteCreationResult );
} );To show one loading for all the work.
|
|
||
| case 'processing': | ||
| if ( providedDependencies?.goToHome && providedDependencies?.siteSlug ) { | ||
| return window.location.replace( | ||
| addQueryArgs( `/home/${ siteId ?? providedDependencies?.siteSlug }`, { |
There was a problem hiding this comment.
We should use siteId from providedDependencies here.
| } | ||
|
|
||
| if ( siteId ) { | ||
| return `/setup/${ flow }/launchpad?siteSlug=${ siteSlug }&siteId=${ siteId }`; | ||
| redirectURL = `/setup/${ flow }/launchpad?siteSlug=${ siteSlug }&siteId=${ siteId }`; |
There was a problem hiding this comment.
Maybe use addQueryArgs here as well?
…components/help-center/async.tsx
…hooks/use-launchpad-decider/index.tsx
| const launchpadUrl = `/setup/${ flowName }/launchpad?siteSlug=${ providedDependencies.siteSlug }`; | ||
|
|
||
| switch ( _currentStep ) { | ||
| case 'intro': | ||
| return navigate( 'newsletterSetup' ); | ||
|
|
||
| case 'newsletterSetup': | ||
| set( 'newsletterSetup', providedDependencies ); |
There was a problem hiding this comment.
If we need to set( STEP_SLUG, providedDependencies ) for every step, could this be something that the Stepper framework automatically takes care of?
There was a problem hiding this comment.
I definitely considered this, but it degrades the clarify of Stepper. Stepper now doesn't manage any state you're not aware of (as a flow owner) so the data flow is quite intuitive. Also, I realized yesterday this hook has a flaw; step slugs are not unique, I'm working on an alternative identifier of the step to use in this hook.
My opinion on not auto calling set is not strong though, especially if we rename submit to updateFlowStateAndSubmit or such.
There was a problem hiding this comment.
I realized yesterday this hook has a flaw; step slugs are not unique, I'm working on an alternative identifier of the step to use in this hook.
That is something that I noticed too (and just discussed with @tyxla earlier today) — could we implement a runtime check when the flow is initialised, that checks if all the steps in a flow have different slugs, and otherwise throws / shows an error? I guess that would also prevent the need for adding an alternative identifier.
There was a problem hiding this comment.
Re. auto-calling set: I see your point about having flows as explicit as possible. On the other end, relying on the user to reliably call set( STEP_SLUG, providedDependencies ) could also lead to unexpected consequences, on top of making the code more verbose. Maybe the right balance is that the framework automatically calls set for the standard provided deps, and then flow authors can manually call set for any additional dep? Excuses if I'm not making sense, but definitely interesting in learning more about the needs and potential solutions
There was a problem hiding this comment.
Great points. Auto-calling set will also make migrating flows much easier.
could we implement a runtime check when the flow is initialised
I'm not worried about flows having steps with non-distinct slugs; I'm really invested into making this hook as strongly typed as possible. And currently it uses the step slug to infer the type of the value it should accept. If the slugs are not unique, we need to find another way to infer the type.
Currently, we have a manifest of key => type here. The keys represent step slugs, and the values are the types of the steps. This is still not ideal because the types are arbitrary and function as hints for linitng, essentially.
I'm considering making another hook (I want to keep this one because it's great for generic state), the other hook would be useStepState and it would accept the whole Step object as key.
Something like
const { setStep, getStep } = useStepState();
setStep( STEPS.INTRO, yourValue );The great thing about this is that it will infer the types from the step without any manifests, but the issue I'm stuck with is that this key is not serializable, and thus we can't persist it. I'm trying to come up with a way to serialize the step object in a deserializable way.
| ) }?redirect_to=${ encodeURIComponent( launchpadUrl ) }&signup=1` | ||
| // Replace the processing step with checkout step, so going back goes to Plans. | ||
| return window.location.replace( | ||
| `/checkout/${ encodeURIComponent( siteFragment ) }?redirect_to=${ encodeURIComponent( |
There was a problem hiding this comment.
This checkout URL will cause a shopping cart error because the siteFragment is being picked up as a possible product to add to the cart on the checkout page.
getProductSlugFromContext() Is triggered here and it attempts to add the siteFragment as another product. If it's possible to replace the siteFragment with siteSlug instead, that will solve the issue. If we need siteFragment here ( appears as siteId when I debug) then we can modify the getProductSlugFromContext function.
alshakero
changed the title
Related to p1738937572081439-slack-C02T4NVL4JJ
Proposed Changes
Why are these changes being made?
Testing Instructions
/setup/example.sessionIDquery parameter is added to the URL (should be two random characters).Non-standard but important tests