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
Add upgrade section in Jetpack plans page #52381
Conversation
Here is how your PR affects size of JS and CSS bundles shipped to the user's browser: App Entrypoints (~21 bytes removed 📉 [gzipped])
Common code that is always downloaded and parsed every time the app is loaded, no matter which route is used. Sections (~260 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 (~3 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. |
@@ -61,6 +61,8 @@ export const PERFORMANCE = 'performance'; | |||
export const SECURITY = 'security'; | |||
export const PLAN_COMPARISON_PAGE = 'https://jetpack.com/features/comparison/'; | |||
|
|||
export const INTRO_PRICING_DISCOUNT_PERCENTAGE = 40; |
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.
Extracting as a constant since it's used in several places.
components: { | ||
product: ( | ||
<span className="plan-upgrade__product"> | ||
{ newItems.reduce( ( result, { displayName }, index ) => { |
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.
Separate names by commas and a &. displayName
is not necessarily a string, so using join
wasn't possible.
</p> | ||
<ul className="plan-upgrade__list"> | ||
<li className="plan-upgrade__legacy-item"> | ||
<ProductCard |
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 will be replaced by a legacy product card.
margin-bottom: 6rem; | ||
} | ||
|
||
.with-single-reco & { |
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.
When only one product is recommended, the breakpoint should be smaller.
I'm not thrilled by these style duplicates, but I haven't found a better solution yet.
/** | ||
* External dependencies | ||
*/ | ||
import isJetpackPurchsableItem from '@automattic/calypso-products/src/is-jetpack-purchasable-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.
There is a small typo in isJetpackPurchsableItem
.
<li className="plan-upgrade__new-items"> | ||
<ul className="plan-upgrade__new-items-list"> | ||
{ newItems.map( ( product ) => ( | ||
<li key={ product.iconSlug }> |
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.
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.
Do you remember which values you used for the products, @rcanepa? I wasn't able to reproduce this.
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 used ?compare_plans=jetpack_premium,jetpack_security_daily,jetpack_backup_realtime
.
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 is really strange. It only happens to me with these specific products, and the debugger clearly shows they have different keys (at no point are they undefined) 🤔 .
const StandardPlansHeader = ( { context }: { context: PageJS.Context } ) => ( | ||
<> | ||
<FormattedHeader headerText={ translate( 'Plans' ) } align="left" brandFont /> | ||
<PlansNavigation path={ '/plans' } /> | ||
<h2 className="jetpack-plans__pricing-header"> | ||
{ preventWidows( | ||
translate( 'Security, performance, and marketing tools made for WordPress' ) | ||
) } | ||
</h2> | ||
{ ! getPlanRecommendationFromContext( context ) && ( | ||
<h2 className="jetpack-plans__pricing-header"> | ||
{ preventWidows( | ||
translate( 'Security, performance, and marketing tools made for WordPress' ) | ||
) } | ||
</h2> | ||
) } |
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 purpose of getPlanRecommendationFromContext
was hard to understand. Maybe we could rename it to something like shouldShowPlansRecommendation
or something like that, and compute that value on the parent component. I don't see a strong reason to pass the whole context object when we can accomplish the same just by passing a boolean.
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.
Fair point. I'll update that.
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.
LGTM. It works and looks as expected.
I left a couple of non-blocking comments. We can definitely improve them in a follow-up PR.
Also, I noticed that when there are two or more plans being recommended, their cards don't have the same width.
I think this can be solved by adding flex: 1 1 100%;
to each product card since the parent container is a flex container.
Awesome work!
Good catch! Thank you. |
bd7757e
to
953275c
Compare
This Pull Request is now available for translation here: https://translate.wordpress.com/deliverables/5809392 Hi @monsieur-z, could you please edit the description of this PR and add a screenshot for our translators? Ideally it'd include all of the following strings:
Thank you in advance! |
Translation for this Pull Request has now been finished. |
Changes proposed in this Pull Request
This PR adds an upgrade section to the plans page, when the
compare_plans
query parameter is set with proper values.Fixes 1200196139286276-as-1200247432598890
Implementation notes
These tasks will be dealt with in separate PRs:
plan-upgrade
folderTesting instructions
Prerequisite
Regression testing
/plans/:site
and check that it looks and behaves as in productionUpgrade section
plans/:site?compare_plans=:legacy-slug,:new-slug
, where:legacy-slug
is the slug of a Jetpack legacy plan, and:new-slug
: the slug of a currently available Jetpack product?compare_plans=:legacy-slug,:new-slug,:new-slug
)compare_plans
parameter, and check that you see the regular plans pageScreenshots
One recommended product
Screen.Recording.2021-04-28.at.8.58.13.AM.mov
Two recommended products
Screen.Recording.2021-04-28.at.8.57.27.AM.mov