-
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
Storage Add-ons: Expose add-on upsells to plans page #83005
Conversation
Here is how your PR affects size of JS and CSS bundles shipped to the user's browser: Sections (~498 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 (~555 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. |
22ceae6
to
9bf387f
Compare
This PR is almost ready for review -- I'm still wrangling with some odd caching logic 🤔 |
c93a5a5
to
f405ccf
Compare
@@ -10,20 +11,23 @@ import { getSelectedSite } from 'calypso/state/ui/selectors'; | |||
|
|||
const useAddOnCheckoutLink = (): ( ( addOnSlug: string, quantity?: number ) => string ) => { | |||
const selectedSite = useSelector( getSelectedSite ); | |||
const checkoutLinkCallback = useCallback( |
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 only change made in this file is the usage of useCallback
to align with the 2023 plans-grid optimization efforts p1697705360647249/1697700875.046909-slack-CV2TX2PAN
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.
Hmm I don't think this needs to return a callback at all. We should probably instead be passing in #83005 (comment)selectedSiteSlug
and return the checkout link directly from the hook.
buttonText = translate( 'Get Essential', { textOnly: true } ); | ||
} | ||
return renderedGridPlans.map( | ||
( { planSlug, availableForPurchase, features: { storageOptions } } ) => { |
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.
Note:
Changes for L368-420 are caused by auto-formatting. The only meaningful update here is that we destructure the storageOptions
parameter and pass it to PlanFeatures2023GridActions
a5e2c92
to
8a70647
Compare
@@ -25,6 +25,7 @@ import { | |||
isEcommercePlan, | |||
TYPE_P2_PLUS, | |||
} from '@automattic/calypso-products'; | |||
import useAddOnCheckoutLink from 'calypso/my-sites/add-ons/hooks/use-add-on-checkout-link'; |
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 is a local dependency we are introducing here. Let's either move it to a respective package and import from there or surface it differently into this hook :-)
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.s. looking at the code for useAddOnCheckoutLink
, it should be easy to just copy over into a respective package by passing siteId in the props.
If that's the direction, would that be packages/data-stores/add-ons
, or is it packages/add-ons
?
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.
Ah thanks for catching this @chriskmnds! My mistake 🙏 I've been digging into the weeds with this PR and was planning to pass it into use-grid-plans as a callback, but it totally slipped my mind
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.
OK. Please read my other comments too. I don't think we should be passing this through as a callback. It complicates things further, and there seems to be a cleaner approach to putting these together.
From the looks of it, it sounds like we can instead compile the storage add-ons with a checkout URL from the existing useStorageAddOns
hook. In which case, it becomes less of an immediate concern to migrate or refactor the useAddOnCheckoutLink
hook.
IF it makes sense. Otherwise, I think moving useAddOnCheckoutLink
to the respective add-ons package might be better than passing in a callback here 🤔
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.
OK. Please read my other comments too.
Yeah I'm seeing the other comments now, and they make sense. 🙂
I'll hold off on responding to individual comments for the time being until I've read everything as a whole
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.
From the looks of it, it sounds like we can instead compile the storage add-ons with a checkout URL from the existing useStorageAddOns hook. In which case, it becomes less of an immediate concern to migrate or refactor the useAddOnCheckoutLink hook.
IF it makes sense. Otherwise, I think moving useAddOnCheckoutLink to the respective add-ons package might be better than passing in a callback here 🤔
I'll move forward with both of these
I like the idea of generating storage add-ons with their respective checkout links for developer convenience, especially because the requisite change is simple and it works well for both existing UI consumers in /add-ons
and /plans
. It also seems like it will maintain a cleaner separation between the plans-grid
and storage add-ons
Beyond that though, moving useAddOnCheckoutLink
also looks like an easy task and gets us one step closer to having add-ons in a self-contained package. It's as good a reason as any to kick off the our migration, so I'll opt to handle that in an immediate follow-up
Edit:
I add checkout links to each storage add-on in this commit 5ca53a6
Edit 2:
Created a PR to migrate the checkout link hook to data-stores/add-ons #83489
@@ -651,6 +657,7 @@ const PlansFeaturesMain = ( { | |||
<QuerySites siteId={ siteId } /> | |||
<QuerySitePlans siteId={ siteId } /> | |||
<QueryActivePromotions /> | |||
<QueryProductsList /> |
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.
Note:
<QueryProductList />
is necessary to generate add-on product data on the /plans
page
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.
Thanks @jeyip for the work. I left some comments that I believe need addressing. Let me know if I can provide more direction.
Especially on introducing new dependencies into the folder (more so into "npm-ready" parts).
client/my-sites/plans-grid/hooks/npm-ready/data-store/use-grid-plans.tsx
Outdated
Show resolved
Hide resolved
client/my-sites/plans-grid/hooks/npm-ready/data-store/use-grid-plans.tsx
Outdated
Show resolved
Hide resolved
client/my-sites/plans-grid/hooks/npm-ready/data-store/use-grid-plans.tsx
Outdated
Show resolved
Hide resolved
@@ -10,20 +11,23 @@ import { getSelectedSite } from 'calypso/state/ui/selectors'; | |||
|
|||
const useAddOnCheckoutLink = (): ( ( addOnSlug: string, quantity?: number ) => string ) => { | |||
const selectedSite = useSelector( getSelectedSite ); | |||
const checkoutLinkCallback = useCallback( |
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.
Hmm I don't think this needs to return a callback at all. We should probably instead be passing in #83005 (comment)selectedSiteSlug
and return the checkout link directly from the hook.
storageOptions?: StorageOption[]; | ||
}; | ||
|
||
export default function useDefaultStorageOption( { storageOptions, storageAddOnsForPlan }: Props ) { |
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.
Let's add a description here.
I haven't grasped entirely the context here, but I presume this is necessary to live under plans-grid
, right?
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.
Added a short description to the hook ac5daa4
@@ -17,5 +17,4 @@ export const isStorageUpgradeableForPlan = ( { | |||
storageOptions.length > 1 && | |||
intervalType === 'yearly' && | |||
showUpgradeableStorage && | |||
isInSignup && | |||
flowName === 'onboarding'; | |||
( flowName === 'onboarding' || ! isInSignup ); |
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 wish we could get rid of flowName
from here, along with isInSignup
(as a general approach to start removing context from plans-grid
). Can we maybe compile these into a property and pass into the grid? 🤔
Something for a follow-up perhaps.
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.
as a general approach to start removing context from plans-grid
Gotcha 👍
Yeah I'm happy to consolidate these properties. For a bit more context, isInSignup
and flowName
are temporary and will disappear entirely from this condition once we release add-on upsells to the rest of the onboarding flows.
Edit:
Will handle this in a follow-up :D
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.
Thanks @jeyip for the work. I left some comments that I believe need addressing. Let me know if I can provide more direction.
Especially on introducing new dependencies into the folder (more so into "npm-ready" parts).
(oops, looks like I clicked on "approve changes" previously 😬 )
Thanks for the speedy review @chriskmnds! Reading and responding to your comments now 😎 I'm also going to take a beat, step away for a bit, and re-review my code in an hour or two to make sure I haven't missed anything else. I think I've tunnel visioned a bit while working on this PR 😵💫 |
Thanks. My comments may feel a bit "all over the place". I kinda formed my thinking through commenting :D Happy to take a look when back on Thursday/Friday or bounce more ideas. |
No worries about forming thoughts through comments -- It's all very helpful! And again, thanks for the speedy review. Yeah I'm noticing context from some later comments informing the discussion in earlier comments, so I'll take a look at all of your feedback as a whole first instead of responding to each idea individually.
Sounds great! Enjoy your AFK :D Edit: Moved this PR into "Needs Author Reply" and working on the suggested changes shortly |
nothing critical. we've discussed the important bit about the dependency. the rest I think are down to taste/cleanup (I got off the wrong assumption at one point at least :)
5ca53a6
to
3e5c14e
Compare
d692b23
to
96b3ae1
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.
Tests out as outlined. I think quite impressive how nicely aligned everything is with the NPM migration work. also admire your approach and attitude toward comments and follow-ups! :D
A couple of things I notice, probably unrelated to this PR:
-
When I change between monthly/yearly plans when visiting with a free site, it clears the previous selections: so going into "yearly" tab, selecting a storage add-on, then clicking "monthly" in the toggle, then back to "yearly", the selections get cleared (and prices might flicker on a slower browser between previously updated/higher price and the default).
-
Verify that, when a storage add-on is purchased, it is rendered in the dropdown as the default on initial page load ( for recently purchased add-ons, stale data may be presented for a minute or two until updated. This same behavior is seen in the /add-ons page )
So I confirm this, but also notice this feels a bit unclear to me. I upgraded a site from Personal to Business and purchased an additional +50GB space (so made the 100GB purchase). Now the dropdown always shows 100 as the default selected value in the Spotlight plan, which feels a little weird. I would expect the two (current plan's value and upgrades) to be separated a little better in the Spotlight plan i.e. it is not clear what that 100 refers to (especially since now my "plan" has a 100GB storage). Probably just a consequence to the two concepts being separated. 🤷♂️
-
Verify that, after a storage add-on has been purchased, selecting a lower tier storage add-on option disables the plan's CTA and asks the user to contact support
Confirmed, but similar impressions to the above. It feels a little cryptic/hidden maybe.
-
on checkout, if I change my selection to 2 years for the plan, the storage add-on stays for a single year. Not sure if that's confusing. It might make sense is the user could change the storage add-on's periodicity at the same time too.
if ( freePlan ) { | ||
if ( | ||
freePlan || | ||
( storageAddOnsForPlan && ! canPurchaseStorageAddOns && nonDefaultStorageOptionSelected ) |
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.
Just a thought. Since we use this condition (canPurchaseStorageAddOns && nonDefaultStorageOptionSelected
) at multiple places, would it make sense to combine into one? Not sure how we'd name that though :D
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.
Hey apologies if I'm not seeing it, but one of the conditions is canPurchaseStorageAddOns && nonDefaultStorageOptionSelected
and the other is ! canPurchaseStorageAddOns && nonDefaultStorageOptionSelected
.
I'm not seeing how they can be combined 😅
Recent work on storage add-ons has definitely felt more and more aligned with your 2023 plans pricing page work thanks to your help 😎 I really appreciate the feedback you've been providing
Good catch -- this is definitely worth a look 🤔 Edit: Fixed this in f467bd9
This is a really interesting observation. I wonder if there's a way we can make it clearer to users that a given storage add-on option is what's currently applied to their site as opposed to an upgrade that can be purchased. Let me touch base with @vinimotaa and it's something that we can focus on in a follow-up
Unfortunately only yearly terms were implemented for storage add-on upgrades https://github.com/Automattic/martech/issues/1907#issuecomment-1662901136, and if I recall correctly, it was for the sake of development velocity. The analogy given by Even was that of purchased domains alongside a 3-year plan, and the idea that said domain isn't registered for 3 years, but for 1 year. With that being said, I do agree that once we see how popular these add-ons might or might not be ( and how much further effort we'd like to invest), it would make a lot of sense to provide longer terms. |
The product list is needed to derive information about storage add-ons on the /plans page
97f35d9
to
7a25662
Compare
Add checkout functionality to spotlight plan Fix plans features main tests Fix type errors Refactor grid plans and default storage options Prevent purchased add-ons from being added to cart Remove unnecessary usePastBillingTransactions hook Add checkout link to storage add-ons data structure Add description to use default storage option hook Display prices for all add-on options
7a25662
to
f467bd9
Compare
4773d1a
to
8ff5ed5
Compare
* Expose add-on upsells to plans page * Add selected storage add-on to checkout cart * Add storage add-on upsells to spotlight plans * Query product list on load The product list is needed to derive information about storage add-ons on the /plans page * Display purchased or default storage option on load
Related to https://github.com/Automattic/martech/issues/2023
Proposed Changes
Important:
/plans
page after recently purchasing a storage add-on. Normally the storage add-on dropdown will default to the most recently purchased plan. For a few minutes after a recent purchase though, it will not. This behavior is what we see in the/add-ons
page as well.GIFs
Testing Instructions
Pre-merge Checklist