-
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
Moves design picker after the goals step #97383
Conversation
Jetpack Cloud live (direct link)
Automattic for Agencies live (direct link)
|
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 |
Here is how your PR affects size of JS and CSS bundles shipped to the user's browser: Sections (~29 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 (~39 bytes removed 📉 [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. |
e21ac6f
to
ececb82
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.
@p-jackson I noticed this TODO item in the PR description
TODO Removes design picker from site-setup
I'm a bit confused on what this means?
I think this PR is good to mark as "Ready for review" as it fulfills the title of "moves design picker after the goals step."
Upcoming work:
- the broken layout that we might/might not address per the prototype
- make sure the "Continue" or "Unlock theme" CTA works
"Continue" works for me. "Unlock theme" doesn't, and it's because we have some site slug gating within diff --git a/client/landing/stepper/declarative-flow/internals/steps-repository/design-setup/unified-design-picker.tsx b/client/landing/stepper/declarative-flow/internals/steps-repository/design-setup/unified-design-picker.tsx
index 29df82da807..782df7dd6db 100644
--- a/client/landing/stepper/declarative-flow/internals/steps-repository/design-setup/unified-design-picker.tsx
+++ b/client/landing/stepper/declarative-flow/internals/steps-repository/design-setup/unified-design-picker.tsx
@@ -532,26 +532,30 @@ const UnifiedDesignPickerStep: Step = ( { navigation, flow, stepName } ) => {
forceRedirection: true,
} );
}
- function handleCheckout() {
+
+ function goToNextStep() {
+ if ( ! wpcomSiteSlug && ! siteSlug ) {
+ return pickDesign();
+ }
+
+ navigateToCheckout();
+ }
+
+ function handleCheckoutModalConfirmation() {
recordTracksEvent( 'calypso_signup_design_upgrade_modal_checkout_button_click', {
theme: selectedDesign?.slug,
theme_tier: selectedDesign?.design_tier,
is_externally_managed: selectedDesign?.is_externally_managed,
} );
- if ( siteSlugOrId ) {
- // We want to display the Eligibility Modal only for externally managed themes
- // and when no domain was purchased yet.
- if (
- selectedDesign?.is_externally_managed &&
- hasEligibilityMessages &&
- ! didPurchaseDomain
- ) {
- setShowEligibility( true );
- } else {
- navigateToCheckout();
- }
+ // We want to display the Eligibility Modal only for externally managed themes
+ // and when no domain was purchased yet.
+ if ( selectedDesign?.is_externally_managed && hasEligibilityMessages && ! didPurchaseDomain ) {
+ setShowEligibility( true );
+ } else {
+ goToNextStep();
}
+
setShowUpgradeModal( false );
}
@@ -698,6 +702,7 @@ const UnifiedDesignPickerStep: Step = ( { navigation, flow, stepName } ) => {
{
selectedDesign: _selectedDesign,
selectedSiteCategory: categorization.selections?.join( ',' ),
+ plan: requiredPlanSlug,
},
{ ...( positionIndex >= 0 && { position_index: positionIndex } ) }
);
@@ -833,7 +838,7 @@ const UnifiedDesignPickerStep: Step = ( { navigation, flow, stepName } ) => {
marketplaceProduct={ selectedMarketplaceProduct }
requiredPlan={ requiredPlanSlug }
closeModal={ closeUpgradeModal }
- checkout={ handleCheckout }
+ checkout={ handleCheckoutModalConfirmation }
/>
) }
<QueryEligibility siteId={ site?.ID } />
@@ -849,7 +854,7 @@ const UnifiedDesignPickerStep: Step = ( { navigation, flow, stepName } ) => {
setShowEligibility( false );
} }
handleContinue={ () => {
- navigateToCheckout();
+ goToNextStep();
setShowEligibility( false );
} }
/>
diff --git a/client/landing/stepper/declarative-flow/onboarding.ts b/client/landing/stepper/declarative-flow/onboarding.ts
index 313dec5cea8..0d297d2ea3c 100644
--- a/client/landing/stepper/declarative-flow/onboarding.ts
+++ b/client/landing/stepper/declarative-flow/onboarding.ts
@@ -128,7 +128,12 @@ const onboarding: Flow = {
}
case 'designSetup': {
- const { selectedDesign: _selectedDesign } = providedDependencies;
+ const { selectedDesign: _selectedDesign, plan } = providedDependencies;
+
+ if ( plan ) {
+ setPlanCartItem( plan );
+ }
+
if ( isAssemblerDesign( _selectedDesign as Design ) && isAssemblerSupported() ) {
return navigate( 'pattern-assembler' );
}
|
@candy02058912 it just means that the current state of the PR means users see the design picker twice. Once before account creation, then as they continue through the flow, after site creation and checkout, they see the design picker again. They should only see it once: before account creation. |
c74a460
to
2bc98c6
Compare
@zaguiini tested those changes, and it unblocks the theme selection; I was wondering if we'd like to add the snippet to a separate PR and set it to merge to this branch so people can check out to that branch and review it more easily if we don't want to make this PR too large. |
I think that's a more sensible decision @candy02058912 since these two are related but I haven't thoroughly tested my snippet, so we'll probably want to give it more attention. |
I'm getting redirected to
|
Scratch that. See p1734385797588649/1734385074.248909-slack-C085HCWCEDN.
Additionally to the snippet shared above, I tried setting the destination as But it didn't work out. After checkout, it redirected me to my site without my theme/style variation. I was talking with @p-jackson about this, and we'll need to briefly redirect to Considering the user already has a site by the time we call the If we want to test only the happy path, another path @p-jackson suggested was to store their options as part of On a different note, we also need to take into account additional marketplace products required for specific themes. |
Pinging @arthur791004 in case you want to pick this up. |
case 'designSetup': { | ||
const { selectedDesign: _selectedDesign } = providedDependencies; | ||
if ( isAssemblerDesign( _selectedDesign as Design ) && isAssemblerSupported() ) { | ||
return navigate( 'pattern-assembler' ); |
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.
The pattern assembler has been sunset
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.
Removed it 👍
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 is good to merge as it isn't reflected in production, and we'll follow up with #97540
Warning
DO NOT MERGE UNTIL D168479-code HAS BEEN MERGEDMerged via: Automattic/wpcom/pull/168479
Related to #97208
Proposed Changes
onboarding
site-setup
in Remove design picker from site-setup flow #97540theme.picker.mp4
Why are these changes being made?
Testing Instructions
Apply the patch D168479-codeflags=onboarding/goals-first
Pre-merge Checklist