-
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
Plans (grid): Disable term-savings price display if another discount is active (intro/coupon/etc.) #96631
Plans (grid): Disable term-savings price display if another discount is active (intro/coupon/etc.) #96631
Conversation
Jetpack Cloud live (direct link)
Automattic for Agencies live (direct link)
|
35896ac
to
f53e700
Compare
f73ba06
to
c8e0442
Compare
@jeyip This is a pretty insane change. If this is where we want things to head, then I'll continue pushing through more surrounding refactors. I think this is where we want things to head, conceptually. cc @southp as something to keep an eye on. The primary consideration going forward should on maintaining the memoization of the affected hooks. Otherwise, it literally can be 4x the processing on any interaction (if not already - needs to be tested). 🤷 |
Well, I'm going to experiment with another approach. This feels a little over the line. |
Thanks for exploring this change. Pretty unfortunate that it gets crazy. I haven't reviewed the code yet, but I'll take a closer look before I sign off |
c8e0442
to
d196100
Compare
Just wrapped up work on storage add-ons related to this experiment all ready for review ( Plans: Make storage selection reflection in header price optional, Plans: Move useStorageAddOns logic selection in pricing meta hook ). Starting a deep dive into this PR today, and alternative approaches we might consider, especially given your concerns voiced here |
b67e8f2
to
1fcb677
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.
Ah. After digging into the code a bit more, I understand the concern. Summarizing to make sure I understand correctly, but feel free to chime in with any important info I'm missing.
Whereas previously we were retrieving and joining plans data only for the term being rendered ( monthly, or annual, or 2year, etc. ), we are now doing so for all billing terms in the useGridPlans
hooks. Besides the added code complexity, we're also seeing O(n) functions become O(n^2) here and here
We're doing so because we'd like to calculate whether or not special offers, coupons, or discounts are visible to any plan on any term, at which point we can decide whether or not we want to render term savings.
I'll start thinking about alternate approaches as well.
@jeyip Thanks for rebasing. I'll dive back into this and see what else I can come up.
Your assessment is generally correct, although complexity is still linear, just Having to loop through all the plans that we may "potentially" render is inevitable for the logic we want to introduce. That's essentially 1:1 to what we intend to achieve. The alternatives I'm thinking of are more about reducing how much we touch when doing so, creating additional structure from indexing on
|
1fcb677
to
9512296
Compare
const usePlanSlugsForAllDisplayedIntervals = () => { | ||
const currentPlanSlugs = gridPlansForFeaturesGrid?.map( ( { planSlug } ) => planSlug ); | ||
|
||
return currentPlanSlugs?.flatMap( ( planSlug ) => | ||
filteredDisplayedIntervals | ||
.map( ( term ) => | ||
getPlanSlugForTermVariant( planSlug, URL_FRIENDLY_TERMS_MAPPING[ term ] ) | ||
) | ||
.filter( ( planSlug ) => planSlug !== undefined ) | ||
); | ||
}; | ||
|
||
const pricingForAllDisplayedIntervals = Plans.usePricingMetaForGridPlans( { | ||
planSlugs: usePlanSlugsForAllDisplayedIntervals() ?? [], | ||
storageAddOns, | ||
coupon, | ||
siteId, | ||
useCheckPlanAvailabilityForPurchase, | ||
} ); | ||
|
||
const enableTermSavingsPriceDisplay = useMemo( () => { | ||
const isAnyGridPlanDiscounted = Object.values( pricingForAllDisplayedIntervals ?? {} ).reduce( | ||
( isDiscounted, { discountedPrice, introOffer } ) => { | ||
const hasDiscount = | ||
'number' === typeof discountedPrice.monthly || | ||
( introOffer && ! introOffer.isOfferComplete ); | ||
return isDiscounted || !! hasDiscount; | ||
}, | ||
false | ||
); | ||
|
||
if ( isAnyGridPlanDiscounted ) { | ||
return false; | ||
} | ||
|
||
return ( | ||
( isEnabled( 'plans/term-savings-price-display' ) || | ||
longerPlanTermDefaultExperiment.isEligibleForTermSavings ) && | ||
isInSignup | ||
); | ||
}, [ | ||
pricingForAllDisplayedIntervals, | ||
longerPlanTermDefaultExperiment.isEligibleForTermSavings, | ||
isInSignup, | ||
] ); | ||
|
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.
@jeyip this is one of the alternatives I had in mind. Does it look more sane from previously?
Complexity is still of the same order (processing all plans), but a smaller surface area of the changes (not touching hooks, etc.). Next step would be to clean this up into a separate hook so it doesn't live like this in the component.
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 one of the alternatives I had in mind. Does it look more sane from previously?
From where it was before, the smaller surface area is absolutely better 👍
Currently reading the code to gauge my understanding.
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.
Currently reading the code to gauge my understanding.
Yes this is significantly more easy to follow 🎉
Next step would be to clean this up into a separate hook so it doesn't live like this in the component.
Totally agree on encapsulating this complexity.
I suspect you might've already thought about this, but it also seems like an eventual refactor ( doesn't have to be done immediately imo ) would be for the new hook to handle isAnyPlanPriceDiscounted
logic in the Header-Price
component.
wp-calypso/packages/plans-grid-next/src/components/shared/header-price/index.tsx
Lines 73 to 97 in 63d3c4a
const termVariantPricing = Plans.usePricingMetaForGridPlans( { | |
planSlugs: termVariantPlanSlug ? [ termVariantPlanSlug ] : [], | |
storageAddOns, | |
coupon, | |
siteId, | |
useCheckPlanAvailabilityForPurchase: helpers?.useCheckPlanAvailabilityForPurchase, | |
} )?.[ termVariantPlanSlug ?? '' ]; | |
const termVariantPrice = | |
termVariantPricing?.discountedPrice.monthly ?? termVariantPricing?.originalPrice.monthly ?? 0; | |
const planPrice = discountedPrice.monthly ?? originalPrice.monthly ?? 0; | |
const savings = | |
termVariantPrice > planPrice | |
? Math.floor( ( ( termVariantPrice - planPrice ) / termVariantPrice ) * 100 ) | |
: 0; | |
useEffect( () => { | |
if ( | |
isGridPlanOneTimeDiscounted || | |
isGridPlanOnIntroOffer || | |
( enableTermSavingsPriceDisplay && savings ) | |
) { | |
setIsAnyPlanPriceDiscounted( true ); | |
} | |
}, [ |
I understand that they are distinct concepts, but it seems like we could generate info for isAnyPlanPriceDiscounted
while we calculate isAnyGridPlanDiscounted
. Curious about your thoughts though.
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.
Ok - so I moved it now to a separate hook.
I suspect you might've already thought about this, but it also seems like an eventual refactor ( doesn't have to be done immediately imo ) would be for the new hook to handle isAnyPlanPriceDiscounted logic in the Header-Price component.
Indeed, I had the same thinking. However, we can maybe excuse the two co-existing in that they serve different purposes - one being a header UI concern (that the header price has indeed rendered a discounted price), the other being a general flag into the system/app to enable/disable other things. I think a good refactor might be to turn it into a hook in the plans data-store. I will explore that in a follow-up.
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.
Ok - so I moved it now to a separate hook.
Wonderful! Now we can also explore any necessary performance optimizations in one place ( if necessary ).
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.
Awesome. I'll create an issue for the above in a bit.
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.
^ #97132
This PR modifies the release build for the following Calypso Apps: For info about this notification, see here: PCYsg-OT6-p2
To test WordPress.com changes, run |
Here is how your PR affects size of JS and CSS bundles shipped to the user's browser: Sections (~155 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 (~155 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. |
Hey @chriskmnds thanks for pushing changes. I'm being pulled away to take a look at #96936 (comment), but I'll take a look at this PR shortly Edit: Okay taking a look now |
@@ -754,6 +757,52 @@ const PlansFeaturesMain = ( { | |||
</div> | |||
); | |||
|
|||
const usePlanSlugsForAllDisplayedIntervals = () => { |
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 / Nitpick
:
I'm seeing that we have two conventional ways of describing the length of a plan. Just to confirm my understanding, is the difference between "Displayed Interval" and "term" that the former describes a UI concern while the other describes a business concern?
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 one refers to the intervals in the toggle and the URL parameters, the other to the plan term/period.
64d369f
to
7de9c28
Compare
770320d
to
8010090
Compare
@jeyip I think we are ready here to test/deploy. I just can't figure out the type error in the unit tests, where it comes from 🤔 |
It looks like f38a86a fixed it unless you're concerned about the |
Testing this PR shortly |
TestingRequirementsOne key concern for us was performance. I ran non-scientific performance evaluations with google chrome dev tools. Note that these metrics will be slower due to the nature of executing code simultaneously with chrome's inspector. Real world interaction times will generally be far below 2.5 seconds.
Example chart from TrunkAfter three trials, scripting times were 2683 ms, 2623 ms, and 2553 ms, with an average of 2619ms. Example Chart with this PRAfter three trials, scripting times were 2631 ms, 2808 ms, and 2616 ms, with an average of 2685ms. What we're focused on is the scripting time in yellow, and there was a modest 2.5% increase on the average time to completion. For each of the flows listed below:
Flows:
Browsers
Notes
|
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 term dropdown interactions still seem reasonably responsive, and the changes look good to me 👍 I found some strange behavior with currency based special offers mentioned in my testing notes, but that's also present in production.
Thanks @jeyip . I will do deploy now.
Can you port that comment into an issue so we can address it? Feel free to assign me to it. :-) |
Of course -- I'm on it 🙂 Edit: #97145 |
Related to https://github.com/Automattic/martech/issues/3403
Related to #96357
Branched from #96492
Proposed Changes
This is a follow-up to the term-savings price display (see core implementation: #96357) to disable the term-savings display if any plan, across any available/effective term for the rendered grid appears as discounted already.
See related discussion: pdvytD-Se-p2#comment-645. Implementation directly addresses comment pdvytD-Se-p2#comment-650:
Why are these changes being made?
Avoid confusion when plans are discounted while the term-savings display is active. Keep the focus on the effective/promoted discount across the user's interaction with the grid.
See related discussion: pdvytD-Se-p2#comment-645. Implementation directly addresses comment pdvytD-Se-p2#comment-650
Testing Instructions
TBD - WIP
Pre-merge Checklist