-
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
Check the plan on launch site to show domain message or not #90866
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: App Entrypoints (~1 bytes added 📈 [gzipped])
Common code that is always downloaded and parsed every time the app is loaded, no matter which route is used. Sections (~4361 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 (~19 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. |
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.
Nice! I haven't had a chance to fully review and test the proposed changes, but noticed couple smaller things on the initial read through the code. Please see the inline comments below. 🙂
As, I am approaching EOD, I will finish reviewing it in the morning (if nobody gets to it earlier)
client/my-sites/customer-home/components/celebrate-launch-modal.jsx
Outdated
Show resolved
Hide resolved
client/my-sites/customer-home/components/celebrate-launch-modal.jsx
Outdated
Show resolved
Hide resolved
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.
Sharing a couple more thoughts.
client/my-sites/customer-home/components/celebrate-launch-modal.jsx
Outdated
Show resolved
Hide resolved
client/my-sites/customer-home/components/celebrate-launch-modal.jsx
Outdated
Show resolved
Hide resolved
const purchases = useSelector( ( state ) => getSitePurchases( state, site?.ID ) ); | ||
const actualPlanPurchase = | ||
purchases | ||
.filter( | ||
( purchase ) => purchase.productSlug === site?.plan?.product_slug && purchase.active | ||
) | ||
.sort( ( a, b ) => new Date( a.expiryDate ) - new Date( b.expiryDate ) )[ 0 ] || null; | ||
const isA4ASite = actualPlanPurchase?.partnerType === 'a4a_agency'; | ||
const isLoading = useSelector( isFetchingSitePurchases ); | ||
|
||
useEffect( () => { | ||
dispatch( fetchSitePurchases( site?.ID ) ); | ||
}, [ dispatch, site?.ID ] ); | ||
|
||
useEffect( () => { | ||
if ( ! isLoading && ( ! isPaidPlan || purchases.length > 0 ) && ! isInitiallyLoaded ) { | ||
setIsInitiallyLoaded( true ); | ||
} | ||
}, [ isLoading, purchases, isPaidPlan, isInitiallyLoaded ] ); |
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.
Looks like there are selectors (e.g. getSitePlanSlug
) that we can leverage to simplify the code.
const purchases = useSelector( ( state ) => getSitePurchases( state, site?.ID ) ); | |
const actualPlanPurchase = | |
purchases | |
.filter( | |
( purchase ) => purchase.productSlug === site?.plan?.product_slug && purchase.active | |
) | |
.sort( ( a, b ) => new Date( a.expiryDate ) - new Date( b.expiryDate ) )[ 0 ] || null; | |
const isA4ASite = actualPlanPurchase?.partnerType === 'a4a_agency'; | |
const isLoading = useSelector( isFetchingSitePurchases ); | |
useEffect( () => { | |
dispatch( fetchSitePurchases( site?.ID ) ); | |
}, [ dispatch, site?.ID ] ); | |
useEffect( () => { | |
if ( ! isLoading && ( ! isPaidPlan || purchases.length > 0 ) && ! isInitiallyLoaded ) { | |
setIsInitiallyLoaded( true ); | |
} | |
}, [ isLoading, purchases, isPaidPlan, isInitiallyLoaded ] ); | |
const sitePlanSlug = useSelector( ( state ) => getSitePlanSlug( state, site?.ID ) ); | |
const purchases = useSelector( ( state ) => getSitePurchases( state, site?.ID ) ); | |
const actualPlanPurchase = purchases.filter( ( purchase ) => purchase.productSlug === sitePlanSlug ); | |
const isA4ASite = isA4APurchase( actualPlanPurchase[0] ); |
Note that in the above suggestion, I've also introduced a helper function isA4APurchase()
in lib/purchases
to avoid sprinkling the string literal to various parts of the code base.
export function isA4APurchase( purchase?: Purchase ) {
return 'a4a_agency' === purchase?.partnerType;
}
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 like the approach of introducing isA4APurchase
that can be potentially reused later. It simplifies the whole logic as well. 👌🏼
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.
Pretty nice, indeed 👍
@@ -77,7 +98,7 @@ function CelebrateLaunchModal( { setModalIsOpen, site, allDomains } ) { | |||
); | |||
buttonText = translate( 'Claim your free domain' ); | |||
buttonHref = `/domains/add/${ site.slug }`; | |||
} else if ( hasCustomDomain ) { | |||
} else if ( hasCustomDomain || isA4ASite ) { |
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.
Instead of removing the mentions of a custom domain, we can probably updating https://github.com/Automattic/wp-calypso/pull/90866/files#diff-b2a3bdf3b4cec65b339e0803b39186ea50d15861cfa347b97294688a3b89e58eR69 to ensure we render the upsell banner for A4A sites!
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.
Good point 👍
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 |
Related to https://github.com/Automattic/dotcom-forge/issues/7164
Proposed Changes
We need to check the site's current plan when it launches. If the site doesn't include a plan, like when the site was created by A4A, it shouldn't show the "claim your domain" message. This PR makes this change.
Why are these changes being made?
We don't offer a free domain as part of a Creator plan made by A4A. More context: https://github.com/Automattic/automattic-for-agencies-dev/issues/482
Testing Instructions
Instructions to test not A4A sites
http://calypso.localhost:3000/sites
and create a site normally. It would be awesome to test different plans, including Free.You should see different popups depending on the circumstances of the blog:
Instructions to test A4A sites
Pre-merge Checklist