-
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: Show next renewal price for Jetpack introductory offers #71016
Checkout: Show next renewal price for Jetpack introductory offers #71016
Conversation
Here is how your PR affects size of JS and CSS bundles shipped to the user's browser: Sections (~444 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 (~844 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. |
const isJetpackIntroductoryOffer = isJetpackProduct( variant ) && introCount > 0; | ||
const translate = useTranslate(); | ||
const translatedIntroOfferPrice = translate( | ||
' for the first %(introTerm)s then %(formattedRegularPrice)s per %(planTerm)s', |
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 opening space is likely to be forgotten in translations. I'd trim the string and use CSS to handle the spacing.
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.
It may be useful to add comments or an example for translators as well.
@@ -126,6 +126,9 @@ export function useGetProductVariants( | |||
? variant.introductoryOfferPrice | |||
: variant.priceFinal || variant.priceFull; | |||
|
|||
const regularPrice = variant.priceFull ? variant.priceFull : price; | |||
const introIntervalCount = variant.product.introductory_offer?.interval_count ?? 0; | |||
const introIntervalUnit = variant.product.introductory_offer?.interval_unit ?? 'month'; |
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.
If the default month
value is used as is in a string, it should be translatable.
@@ -95,6 +96,23 @@ export const ItemVariantDropDownPrice: FunctionComponent< { | |||
const formattedCompareToPriceForVariantTerm = compareToPriceForVariantTerm | |||
? formatCurrency( compareToPriceForVariantTerm, variant.currency, { stripZeros: true } ) | |||
: undefined; | |||
// Jetpack introductory pricing displays the introductory price for the first term, then the regular price for the remaining term. | |||
const planTerm = variant.termIntervalInDays > 31 ? 'year' : 'month'; |
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.
Values should be translatable since they're used as is in a string.
Though it's not a concern for Jetpack products at the moment, I'm wondering if this could be more future proof. WPcom plans have monthly, annual, and biennial terms I believe. It'd be great if we could support different terms.
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 working on this! I second the caution to make sure this looks good at mobile widths. Trying to fit more things into the variant picker options has been an oft-discussed and very complicated topic (you can read more about some of the attempts in pbOQVh-24K-p2 and its comments). |
My secret tip for testing things like this is to switch my account to use Portuguese. As a language, it has some of the longest words 👍 |
I am not a designer but I like this idea. We could also hide that small subtext at narrow viewport widths. |
From an accessibility standpoint, we should probably make sure the first line (e.g. $120 $1) is not read by screen readers, since it duplicates the information of the second line. Adding |
Ok, I took a crack at this... Since these are new strings I can't see what they'd actually look like in German or PT. I slightly shortened the message for desktops and went with displaying a very short message for mobile: @sirbrillig @mattgawarecki and @michaeldcain Thoughts? |
also noting that I'll refactor this to eliminate some duplication once we settle on what it should say |
Don't feel like my comment should be blocking this PR, but I'm a bit concerned by the size of the text, especially on mobile. I think it'd be okay for |
@@ -6,6 +6,10 @@ export type WPCOMProductVariant = { | |||
price: number; | |||
pricePerMonth: number; | |||
pricePerYear: number; | |||
regularPrice: number; |
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 think we should be more clear about what "regular" means here, either by providing a more clear name and/or adding a comment. There's so many versions of a price we could have. I'm guessing this is in comparison to the introductory offer price? So maybe priceAfterIntroductoryOffer
?
FYI #70452 changed where the variant data comes from (it's now provided as an integer from the server) and unfortunately will affect this PR. 😓 Let me know how I can help. |
Do you mean as in the font size, or the amount of text? |
The font size. |
I'm working on providing more price points directly from the server shortly, so you should be able to find out things like the price of the variant after the introductory offer expires without having to do anything. I'll post an update here when that's ready. |
D95710-code should give you the data you need to continue working on this PR after a rebase. |
4591674
to
d2079e1
Compare
The commit I just pushed should address compatibility with @sirbrillig 's PR. Now on to working on the A11Y/display stuff that came up. |
Ok, for clarification, are mentioning the price at the top of the cart (the one that shows the original price crossed out in the screenshot) Or, the price above the $1 first month part? |
@sirbrillig I see the price text is mis-aligned between the two rows -- is that what you're referring to, or is there something else I'm missing? FWIW, I don't see the same behavior when I check out with Social Basic and Scan in my cart (using this PR's calypso.live link). I can't help but wonder if I'm missing something 🤔 |
The line break happens when you do not have a Jetpack introductory offer and you do have a discount (so you get the "Save X%" text). Maybe try a non-Jetpack site checkout to see it more easily. |
@sirbrillig I see the issue now, and I think I can quickly fix it without interrupting the string freeze. Working on this now. |
This Pull Request is now available for translation here: https://translate.wordpress.com/deliverables/7697916 Thank you @TheSteveK for including a screenshot in the description! This is really helpful for our translators. |
f53f31a
to
7ada6d6
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.
Thanks, @mattgawarecki! That seems to have resolved the issue I saw.
Deploying without translations, per p1672247215519499/1672218240.237819-slack-C03V4PXKUGP |
This Pull Request is now available for translation here: https://translate.wordpress.com/deliverables/7697916 Thank you @TheSteveK for including a screenshot in the description! This is really helpful for our translators. |
By the way, not related to this PR at all but in your screenshots above you can see an example of the help icon not being styled correctly for Jetpack checkout: #70607 |
Translation for this Pull Request has now been finished. |
Proposed Changes
Images for translators:
Desktop:
Mobile:
Testing Instructions
Pre-merge Checklist
Related to #