-
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: Display introductory offer intervals in the cart line items #50494
Checkout: Display introductory offer intervals in the cart line items #50494
Conversation
} | ||
if ( intervalUnit === 'year' ) { | ||
if ( intervalCount === 1 ) { | ||
text = translate( 'Free for first year' ); |
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.
ℹ️ String reuse speeds up translation and improves consistency. The following string might make a good alternative and has already been translated 23 times:
translate( 'First year free' )
ES Score: 6
Here is how your PR affects size of JS and CSS bundles shipped to the user's browser: Sections (~198 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 (~194 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. |
client/my-sites/checkout/composite-checkout/components/wp-order-review-line-items.tsx
Outdated
Show resolved
Hide resolved
a2aa5c6
to
619587b
Compare
client/my-sites/checkout/composite-checkout/components/wp-order-review-line-items.tsx
Outdated
Show resolved
Hide resolved
d303cc0
to
9b6310e
Compare
if ( ! isRenewalItem && serverCartItem.months_per_bill_period === 1 ) { | ||
return translate( 'Monthly subscription' ); | ||
} |
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.
Without this then all the components dependent on the existing of a sublabel won't be rendered (including the discount callouts). This also is helpful to highlight a monthly subscription since our usual billing intervals are annual.
return translate( 'Renewal' ); | ||
} | ||
|
||
return ''; |
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 thought it would be better to separate the default case.
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.
const intervalUnit = product.introductory_offer_terms.interval_unit; | ||
const intervalCount = product.introductory_offer_terms.interval_count; | ||
|
||
if ( product.cost === 0 ) { |
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 might not matter for the case of 0
, but in general it might be safer to use product_cost_integer
instead of cost
, because it will be an integer in the currency's smallest unit rather than a float in a larger unit. For example, the cost
might be 95.25
but the product_cost_integer
would be 9525
. (This is true of all the ..._integer
prices in the cart; I'd like to deprecate the old properties that use floats.)
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 used item_subtotal_integer instead of cost, I noticed it is used in other places as well to determine free line items. Thanks for the suggestion!
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.
Overall the wording looks good to me.
product: ResponseCartProduct; | ||
} ): JSX.Element | null { | ||
const translate = useTranslate(); | ||
let text = String( translate( 'Discount for first period' ) ); |
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.
Nit: we can define text
after the exit condition on the next line. Same probably applies to setting translate
.
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 I could move the variable but I'm not sure about the translate
assignment. I think it's a good practice to always define the hooks at the top, unaffected by conditional statements.
|
||
if ( product.item_subtotal_integer === 0 ) { | ||
if ( intervalUnit === 'month' ) { | ||
if ( intervalCount === 1 ) { |
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.
Shouldn't this use the three-argument form of translate()
? The same applies to all the locations below.
if ( intervalCount === 1 ) { | |
text = translate( | |
'First month free', | |
'First %(numberOfMonths)s months free', | |
{ | |
args: { | |
numberOfMonths: intervalCount, | |
}, | |
count: intervalCount, | |
} | |
); |
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 Pull Request is now available for translation here: https://translate.wordpress.com/deliverables/5587988 Thank you @dylanrevisited 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
Testing instructions