-
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
First iteration of plans store #41856
Conversation
Test live: https://calypso.live/?branch=add/plan-store |
Here is how your PR affects size of JS and CSS bundles shipped to the user's browser: App Entrypoints (~1 bytes removed 📉 [gzipped])
Common code that is always downloaded and parsed every time the app is loaded, no matter which route is used. 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. |
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 seems like a good start to have proper selectors and state handling for the plan selection feature. We just need to preserve the existing functionality so the domain could be set, in the order of importance, from:
- selecting from PlansGrid
- having the plan slug in the URL
- selecting a paid domain
|
||
const hasPaidDomain = useSelect( ( select ) => select( ONBOARD_STORE ).hasPaidDomain() ); | ||
const planPath = usePlanRouteParam(); | ||
const plan = getPlanSlugByPath( planPath ) || ( hasPaidDomain ? defaultPaidPlan : freePlan ); |
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.
By removing this, we are breaking 2 existing functionalities (detailed in #41471 and #41679):
- getting plan from URL.
- replacing Free plan (shown by default) with a paid default version (eg: Personal) when the user picks a paid domain.
I suggest:
- creating a selector for defaultPlan
- calling
useEffect
withplanPath and hasPaidDomain
as dependencies to dispatchsetPlan
only when there is no selected plan.
|
||
const DEFAUlT_STATE: { selectedPlan: Plan; supportedPlans: Array< Plan > } = { | ||
supportedPlans, | ||
selectedPlan: getPlan( supportedPlanSlugs[ 0 ] ), |
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 should leave it undefined
unless there is explicitly set by using /beginner
route or using the PlansGrid.
ffd845b
to
52817a6
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.
🚀
Changes proposed in this Pull Request
This PR is the first iteration for the store of our plans in Gutenboarding.
Note: I put the files under
stores
dir as opposed togutenboarding/lib/plans.ts
for consistency. Files undergutenboarding/lib/plans.ts
can be removed after this.Related to #41787.