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
Offer Reset: create Upsell page #44954
Conversation
Here is how your PR affects size of JS and CSS bundles shipped to the user's browser: Sections (~673 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. 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. |
685d9d0
to
c2ef599
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.
Feedback left. Note this will need a significant rebase when #44927 is merged, so may be worth waiting till afterwards to tackle my feedback 😄
client/my-sites/plans-v2/upsell.tsx
Outdated
const onPurchaseSingleProduct = useCallback( () => { | ||
addItem( jetpackProductItem( productSlug ) ); | ||
goToCheckout(); | ||
}, [ goToCheckout, productSlug ] ); |
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.
Curious why the code uses useCallback
here? Does any of these need memoization?
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.
Not memoization but on every render those functions are going to be recreated making children components re-render even if the rest of the props haven't changed. I'll say that I'm used to do it. On higher components in the components tree could be troublesome because by definition those will have many children.
On the other hand, I don't think having those could hurt. What do you think?
client/my-sites/plans-v2/upsell.tsx
Outdated
mainProduct: mainProductName, | ||
upsellProduct: upsellProductName, | ||
}, | ||
comment: '%s are abbreviated name of product such as Scan or Backup', |
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 be specific as to what %s
the comment is referring to, in case this string ever has something else added.
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.
Yeah, those are mistakes from a copy-paste.
mainProduct: mainProductName, | ||
upsellProduct: upsellProductName, | ||
}, | ||
} |
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.
Needs a comment. Also should the currency symbol be an arg?
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 catch!
client/my-sites/plans-v2/upsell.tsx
Outdated
if ( ! product ) { | ||
// If the product is not valid or there is no upsell product for it, | ||
// send the user to the selector page. | ||
if ( ! mainProduct || ! upsellProduct ) { |
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.
Perhaps the code could instead send the user to the checkout page if there's no upsell product? The current structure of does not check if an upsell exists before sending the user to this 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.
This a very good idea.
client/my-sites/plans-v2/utils.ts
Outdated
@@ -209,6 +210,7 @@ export function itemToSelectorProduct( | |||
productSlug: item.product_slug, | |||
iconSlug: `${ item.product_slug }_v2`, | |||
displayName: getJetpackProductDisplayName( item ), | |||
shortName: getJetpackProductShortName( item ), |
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 type above adds shortName
as required, but it is only implemented on products, not plans. This will cause TS errors and should be fixed, even with an empty string if no such data exists for plans.
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 catch.
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.
Whoops, meant to not approve!
c2ef599
to
9a7fff7
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.
Looks okay! Some minor cleanup comments, but overall looks good. 👍
/> | ||
</Main> | ||
); | ||
}; |
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 for now, but let's re-use the ProductCardWrapper
component soon.
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.
Will do in follow up PR because I we need to update ProductCardWrapper
to make it accept a onCancelClick
and cancelLabel
.
onButtonClick={ onPurchaseBothProducts } | ||
cancelLabel={ translate( 'No, I do not want %s', { | ||
args: [ upsellProductName ], | ||
} ) } |
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 two strings here should have a comment.
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 catch!
This Pull Request is now available for translation here: https://translate.wordpress.com/deliverables/4241654 Thank you @rcanepa for including a screenshot in the description! This is really helpful for our translators. |
This Pull Request is now available for translation here: https://translate.wordpress.com/deliverables/4241654 Thank you @rcanepa for including a screenshot in the description! This is really helpful for our translators. |
Translation for this Pull Request has now been finished. |
Changes proposed in this Pull Request
Implementation notes
Testing instructions
From Jetpack Backup Daily to Jetpack Scan Daily (monthly and yearly)
http://calypso.localhost:3000/plans/:site/jetpack_backup_daily/annual/additions
http://calypso.localhost:3000/plans/:site/jetpack_backup_daily_monthly/monthly/additions
From Jetpack Scan Daily to Jetpack Backup Daily (monthly and yearly)
http://calypso.localhost:3000/plans/:site/jetpack_scan/annual/additions
http://calypso.localhost:3000/plans/:site/jetpack_scan_monthly/monthly/additions
Fixes 1169247016322522-as-1189040767770459
Demo
Jetpack Scan + upsell Jetpack Backup Daily
Jetpack Backup Daily + upsell Jetpack Scan