-
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
Make sure user lands on PatternAssembler after select blank-canvas #71852
Conversation
Here is how your PR affects size of JS and CSS bundles shipped to the user's browser: Sections (~346 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 (~135 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. |
5fbc0db
to
d22b600
Compare
Update: for some reason, the change only work on localhost but not Calypso Live, will need to figuring why... |
Update: the Calypso Live URL require the flag to be persisted on URL, but since the theme showcase requires a redirect, we need to enable the feature via cookie p1673950877830519/1673944921.451349-slack-CRWCHQGUB
JLFccP.mp4 |
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.
Maybe it's my environment, but It didn't work on the first attempt. I'll try again.
@@ -64,7 +79,11 @@ const PatternAssembler: Step = ( { navigation, flow } ) => { | |||
useEffect( () => { | |||
// Require to start the flow from the first step | |||
if ( ! selectedDesign ) { | |||
goToStep?.( 'goals' ); | |||
if ( signupSelectedThemeSlug !== 'blank-canvas-3' ) { |
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.
We can use isBlankCanvasDesign( signupSelectedThemeSlug )
from @automattic/design-picker
.
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.
@miksansegundo signupSelectedThemeSlug
is a string while isBlankCanvasDesign
expects a Design object, so I don't think we can use it like suggested
client/landing/stepper/declarative-flow/internals/steps-repository/pattern-assembler/index.tsx
Outdated
Show resolved
Hide resolved
client/signup/controller.js
Outdated
@@ -295,6 +296,11 @@ export default { | |||
if ( ! isEmpty( additionalDependencies ) ) { | |||
context.store.dispatch( updateDependencies( additionalDependencies ) ); | |||
} | |||
if ( themeParameter ) { | |||
setSignupSelectedThemeSlug( query.theme ); |
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.
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.
I think it would be better to set the query string via the getChecklistThemeDestination
function because that's why we store the themeParameter
in additionalDependencies
(L290 - L294)
See
wp-calypso/client/signup/config/flows.js
Line 115 in bac42aa
return `/setup/site-setup/patternAssembler?siteSlug=${ siteSlug }`; |
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.
Besides, I think it would be better to create a new flow and include only the PA step. It's a bit weird to redirect the user to a certain step of the flow after the signup. What do you think?
For example, podcast flow, free flow, etc
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.
create a new flow and include only the PA step
That would be extremely helpful for #72286 too!
client/signup/controller.js
Outdated
@@ -295,6 +296,11 @@ export default { | |||
if ( ! isEmpty( additionalDependencies ) ) { | |||
context.store.dispatch( updateDependencies( additionalDependencies ) ); | |||
} | |||
if ( themeParameter ) { | |||
setSignupSelectedThemeSlug( query.theme ); |
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.
I think it would be better to set the query string via the getChecklistThemeDestination
function because that's why we store the themeParameter
in additionalDependencies
(L290 - L294)
See
wp-calypso/client/signup/config/flows.js
Line 115 in bac42aa
return `/setup/site-setup/patternAssembler?siteSlug=${ siteSlug }`; |
client/signup/controller.js
Outdated
@@ -295,6 +296,11 @@ export default { | |||
if ( ! isEmpty( additionalDependencies ) ) { | |||
context.store.dispatch( updateDependencies( additionalDependencies ) ); | |||
} | |||
if ( themeParameter ) { | |||
setSignupSelectedThemeSlug( query.theme ); |
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.
Besides, I think it would be better to create a new flow and include only the PA step. It's a bit weird to redirect the user to a certain step of the flow after the signup. What do you think?
For example, podcast flow, free flow, etc
goToStep?.( 'goals' ); | ||
} | ||
// User has selected blank-canvas-3 theme from theme showcase | ||
setSelectedDesign( blankCanvasDesign as Design ); |
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 has to be wrapped inside the else
condition to ensure we won't save the blank canvas design after goToStep
executed
Just noting here that team T-Rex is working on adding a direct link from the theme research results to access the assembler directly. See https://github.com/Automattic/dotcom-forge/issues/1453 Let's keep in mind that the assembler flow won't be not only a site creation flow but also a theme creation flow to design a new homepage on existing sites and creating a new flow with only the assembler step will help to skip tweaks that are only for the site creation use case like the redirection to ensure users have an |
This is brilliant idea, let's see what I can do |
d22b600
to
1dcf389
Compare
The issues of this PR will be addressed on #72747 |
Proposed Changes
Testing Instructions
document.cookie = 'flags=pattern-assembler/logged-out-showcase;max-age=1209600;path=/'
ero26H.mp4
Pre-merge Checklist
Related to #71706