-
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
Checkout: Update G Suite nudge to replace CartData with withShoppingCart #48438
Checkout: Update G Suite nudge to replace CartData with withShoppingCart #48438
Conversation
Here is how your PR affects size of JS and CSS bundles shipped to the user's browser: Sections (~155 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 (~11 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. |
You will be presented with an upsell whenever you purchase a new domain at:
That would be the
Note I'm mentioning this mostly for me, I was trying to make sure I got everything right 😅. Now to answer your question. this nudge was disabled in #39744 so I guess the only way to test it is to force its url as you suggested. |
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.
Code looks good
const getThankYouPageUrlArguments = { | ||
siteSlug: this.props.siteSlug, | ||
receiptId: this.props.receiptId, | ||
cart: this.props.cart, | ||
}; | ||
const url = getThankYouPageUrl( getThankYouPageUrlArguments ); | ||
page.redirect( url ); |
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.
Is this a pretty safe assumption for the redirect? I was looking through the previous handler and it seemed pretty complicated.
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.
getThankYouPageUrl
was created by meticulously going through the redirect portion of handleCheckoutCompleteRedirect
and replicating it, so it's at least as accurate as checkout. (And after this PR, we'll be able to remove handleCheckoutCompleteRedirect
entirely!)
I tried the testing instructions and everything worked with no errors, but there was one minor difference. I started on a site with just a free plan (no purchases). I added a personal plan and a domain to the cart and followed the directions. But at the last step, in checkout, my cart had the gsuite product and the domain, but the plan was removed. This is the opposite of what I thought should be expected, although it makes more sense since you can't have gsuite without a domain. At any rate there were no errors, and I'm pretty sure that's the main thing to be concerned about with this change. |
}; | ||
|
||
removePlanFromCart() { | ||
removePlanFromCart = () => { |
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.
Upon further inspection maybe the plain is supposed to be removed. :)
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.
Yes, I think it is. But I think I actually translated this logic backwards.
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.
But if you still saw the plan removed, that implies that it was happening on the server, so this code may in fact be unnecessary. I'll verify.
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.
You know, I think it is unused because you're not likely to get to this upsell unless the cart is empty (as you've just made a purchase).
…art (#48438) * Switch GSuiteNudge to use withShoppingCart * Wrap GSuiteNudge in CalypsoShoppingCartProvider * Replace handleCheckoutCompleteRedirect in GSuiteNudge * Add isMounted to GSuiteNudge * Fix CalypsoShoppingCartProvider import * Fix isMounted check * Remove removePlanFromCart as the cart will be empty when you arrive
Changes proposed in this Pull Request
This updates the
GSuiteNudge
component to replaceCartData
withuseShoppingCart
. This should help us eventually removeCartData
(see #24019).Testing instructions
I'm not sure how to trigger this upsell naturally. Pinging @stephanethomas for assistance please!Turns out this is disabled.You can force it though by visiting
/checkout/:site/with-gsuite/:domain/:receiptId?
(eg: http://calypso.localhost:3000/checkout/example.com/with-gsuite/example.com)