-
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
Pricing Grid: Fixes billing timeframe for 2Y & 3Y plans #77823
Conversation
Here is how your PR affects size of JS and CSS bundles shipped to the user's browser: Async-loaded Components (~40 bytes removed 📉 [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. |
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 looks like there's overlap between the changes here and the small PR that I created yesterday #77818.
I'm closing my PR in favor of these comprehensive updates
} | ||
} else if ( rawPrice ) { | ||
if ( PLAN_ANNUAL_PERIOD === billingPeriod ) { | ||
return translate( 'per month, %(rawPrice)s billed annually, Excl. Taxes', { |
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.
Non-blocking / Nit
:
I'm noticing a distinct difference in language that we use to describe the time horizon of yearly plans
billed annually / billed every two years / billed every three years
vs.
for the first year / for the first two years / for the first three years
This seems fine to me, but I wanted to make sure that this variation is purposeful.
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 spot! It is purposeful as we want to make the distinction that the discounted price applies for the first year/term only, and then the full price of the plan would apply for subsequent terms.
); | ||
} ); | ||
|
||
test( 'should show full-term discounted price when plan is 3-yearly', () => { |
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.
Non-blocking / Nit
:
Would you mind describing what full-term discounted means? I see it used throughout this part of the codebase and just assume it means a non-monthly discounted plan
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.
Sure! "full-term discounted price" refers to the price for the complete duration of the plan with any applicable discounts. The discount could either be a currency-specific discount or a discount due to the application of pro-rated credits. For example:
"280" is the "per month discounted price", and 3360 is the "full-term discounted price".
originalPrice: 200, | ||
}; | ||
|
||
usePlanPricesDisplay.mockImplementation( jest.fn( () => planPrices ) ); |
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.
Non-blocking / Nit
:
Not necessary to do in this PR. I see we have a beforeEach
, but how would we feel about an afterAll
to to tear down mocks?
afterAll(() => {
jest.clearAllMocks();
});
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.
Just wanted to say that I always appreciate updated specs in PRs 🎉 Code looks good to me. Testing now.
@@ -74,82 +73,69 @@ function usePerMonthDescription( { | |||
}, | |||
} ); | |||
} | |||
return null; |
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.
Non-blocking / Nit
:
Is it worth writing a spec for this null case?
TestingFor the 3Y plan, I didn't know to to set up a triannual toggle. Instead, I manually rendered the text which looked reasonable. Requirements
Browsers
|
This Pull Request is now available for translation here: https://translate.wordpress.com/deliverables/8028803 Thank you @aneeshd16 for including a screenshot in the description! This is really helpful for our translators. |
Translation for this Pull Request has now been finished. |
Fixes Automattic/martech#1811
Proposed Changes
Note: Once the PR is approved, I'll add the
String Freeze
label to send the new text for translations.2Y plans:
3Y plans:
Testing Instructions
/plans
and confirm that the text matches the screenshot./plans
and confirm that the text matches the screenshot.Pre-merge Checklist