-
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: Implement dropdown checkout functionality #81340
Storage Add Ons: Implement dropdown checkout functionality #81340
Conversation
Here is how your PR affects size of JS and CSS bundles shipped to the user's browser: Sections (~598 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 (~148 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. |
@@ -1270,7 +1269,7 @@ export function isNewOrExistingSiteFulfilled( stepName, defaultDependencies, nex | |||
} | |||
} | |||
|
|||
export const buildUpgradeFunction = ( planProps, cartItem ) => { | |||
export const buildUpgradeFunction = ( planProps, cartItem, addOnItem ) => { |
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:
Amendments to this file allow us to add storage add-ons to the shopping cart in signup onboarding flows
Note: There's a delay between when a storage upgrade add-on is purchased and when the purchase is reflected in the |
86e9897
to
fe2ed2a
Compare
ab7ad7b
to
d4e35ef
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.
@jeyip I didn't manage to look into the code deeply today, but the /start
flow works as expected :)
Since we are now restricting it into /start
only, could you help remove all the unncessary part outside of that? That will greatly reduce the required amout of code review and testing :)
@@ -140,7 +140,7 @@ const useAddOns = ( siteId?: number ): ( AddOnMeta | null )[] => { | |||
|
|||
const spaceUpgradesPurchased: number[] = []; | |||
|
|||
if ( filteredTransactions?.length ) { | |||
if ( filteredTransactions?.length && siteId ) { |
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'm not sure I understand why the siteId all of a sudden is needed here. Could you help me understand?
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 for the patience!
This was meant to fix a bug when we call use-add-ons
in the context of onboarding. For some reason, add-on data incorrectly decides that both 50GB add-ons and 100GB add-ons are already purchased, even though no site has been created yet.
To elaborate on why, filteredTransactions
returns transactions for a given site when siteId
exists. When, however, siteId is undefined, filteredTransactions
fetches all transactions for a user, regardless of what site the purchase was made on.
Since I had made some space upgrade add-on purchase in the past on any site, the logic then proceeded to mark add-on data as purchased
wp-calypso/client/my-sites/add-ons/hooks/use-add-ons.ts
Lines 231 to 244 in 426d78e
// if storage add on is already purchased | |
if ( | |
spaceUpgradesPurchased.findIndex( | |
( spaceUpgrade ) => spaceUpgrade === addOn.quantity | |
) >= 0 && | |
product | |
) { | |
return { | |
...addOn, | |
name, | |
description, | |
purchased: true, | |
}; | |
} |
By checking for a truthy siteId
, I intended to only look for space upgrade add-on transactions if the site has already been created ( and we're in the plans page )
With that being said, I wrote this when I was really tired 😴 , so we might consider better inline documentation or exploring alternative ways to address the bug. Ex. It looked like it might be possible to update the params for filterTransactions
such that, when called with an undefined siteId, it returns no transactions, rather than all transactions, but I haven't looked to closely yet
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 for the explanation. It looks like the transaction data is used for filtering out the storage add-ons that have lower volumn than the owned one. Also, how the whole transaction data is fetched and filtered is actually quite non-ideal –– it fetches the whole transaction history and than filter from the client side, while all these should be done from the server side so our users don't have to waste their bandwidth and local computational resources to perform all these. Since correcting the whole thing would be out of scope, let's start by not fetching those transaction data in the onboarding flow since that doesn't make sense. It'd involve some level of decoupling, but we will have a cleaner code without tricky conditions needing explicit documentation :)
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.
e5a8436
to
4a92911
Compare
Working on it now :D |
e7f13f0
to
9527344
Compare
3e7843b
to
1ad269a
Compare
9a05d1b
to
7130bd4
Compare
@jeyip Sorry for being ignorant about the design decisions made in the past, but could you help me point out the reference of showing the standard storage + the add-on storage in the plans grid? I'm asking since I feel this is a bit confusing:
Also cc @gmovr and @dzver here in case you both happen to know this on top of your mind :) |
@southp I don't know on top of my head. My goal is to unblock this task so that it gets shipped, and I'm leaning toward simplification rather than making things more complex. Whatever the design is, it has to show we support space upgrades, and that's pretty much the only solid requirement. @jeyip could you point to the latest design on this? I don't see a link in the PR or the corresponding issue. |
Sure I'd be happy to:
"As the 50 GB and 100GB storage upgrades don't make sense for Free, Personal, and Premium –Simply upgrading to the Business plan would provide 50 GB space much cheaper. I didn't include storage options for those plans on the grid even though they are technically possible." https://github.com/Automattic/martech/issues/1907#issuecomment-1630537955 |
Thanks for pointing out the references for me, @jeyip :) From my view, this design decision works with the dropdowns, since the label and the dropdowns are obviously communicating different things. However, when all that we have is the label, it doesn't work since they look the same but having different meanings. If we still agree to the original decision of rolling the add-on UI out to Does that make sense to you? |
This does 🙂 If we end up changing our minds, we can draft a relatively quick PR to update the storage add-ons label in |
7f500b3
to
8384f7a
Compare
@@ -100,6 +102,7 @@ export interface PlanFeatures2023GridProps { | |||
planTypeSelectorProps: PlanTypeSelectorProps; | |||
// temporary: callback ref to scroll Odie AI Assistant into view once "Compare plans" button is clicked | |||
observableForOdieRef: ( observableElement: Element | null ) => void; | |||
selectedStorageOptions?: SelectedStorageOptionForPlans; |
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.
selectedStorageOptions
prop doesn't seem relevant here and should be removed.
The only relevant type for your case is PlanFeatures2023GridType
below I think
(it's a little confusing, I know. parts that are being cleaned up in other work)
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 for pointing this out! I removed the unnecessary type here d5a8c5c
@@ -111,6 +114,7 @@ interface PlanFeatures2023GridType extends PlanFeatures2023GridProps { | |||
isPlanUpgradeCreditEligible: boolean; | |||
// temporary: element ref to scroll comparison grid into view once "Compare plans" button is clicked | |||
plansComparisonGridRef: ForwardedRef< HTMLDivElement >; | |||
selectedStorageOptions?: SelectedStorageOptionForPlans; |
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.
so this is fine and the only place you need to define this
There was a Additionally, the |
As it exists today, storage labels show the default amount of storage in the pricing grid, even if a storage upgrade add-on is purchased. We ensure that we continue to do so if the storage add-on dropdown is not displayed
Revert isEmpty change Remove unnecessary check for cartItem Fix pro plan upgrade button Fix plan step unit test Fix typescript errors
There was a cartItem dependency for the site-picker step that needed to be updated to cartItems. On closer inspection, however, it appears as if the site-picker step doesn't need the dependency at all. The logic within the site-picker component makes no indication of needing it, so I remove the cartItem dependency instead. Additionally, the plans step in the domain flow was being skipped under certain circumstances, so we update the provided cartItem dependency to cartItems in those circumstances as well.
9da13ef
to
d09c44b
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.
Beautifully done. Thanks for the great effort you've put into the fix, @jeyip . It's impressive to see you go extra miles further tidying the code up while fixing it up.
Also, since @chriskmnds has two PRs based on this PR(#81960 and #82054), I'm deploying this shortly to move things forward :) |
Related to #79041 (comment)
Issue https://github.com/Automattic/martech/issues/2025
Tracking https://github.com/Automattic/martech/issues/2023
Proposed Changes
/start
signup onboarding flow with storage add onsGIFs
To be left as a Follow Up for the Future
Testing Instructions
Pre-merge Checklist