Plans: Header price-discounting refactors

[chriskmnds]

Related to https://github.com/Automattic/martech/issues/3403

Proposed Changes

• Follow-up on Plans: Enable term savings price display - as a grid feature #96357 to improve the logic around discounting price display - in particular, how non-discounted prices are displayed when another discounted price is rendered.
• Explored a couple of ideas, all depicted across the first few distinct commits:
  • use the wpcom-plans-ui wp-data store
  • a localised userEffect hook
  • a header-price context provider
• Header-price context seems to lead to the least amount of ambiguity and code duplication

Why are these changes being made?

The goal is to (1) centralize the logic outlined above, (2) reduce processing, (3) avoid code duplication.

Testing Instructions

• Go to /plans/:site?flags=plans/term-savings-price-display and /start/plans?flags=plans/term-savings-price-display
• Ensure for yearly and above the savings are displayed as per media above
• Try the same with a coupon (need to find one) or active intro offer (need to mock/code it) and ensure those discounts render instead for the respective plans

Pre-merge Checklist

• Has the general commit checklist been followed? (PCYsg-hS-p2)
• Have you written new tests for your changes?
• Have you tested the feature in Simple (P9HQHe-k8-p2), Atomic (P9HQHe-jW-p2), and self-hosted Jetpack sites (PCYsg-g6b-p2)?
• Have you checked for TypeScript, React or other console errors?
• Have you used memoizing on expensive computations? More info in Memoizing with create-selector and Using memoizing selectors and Our Approach to Data
• Have we added the "[Status] String Freeze" label as soon as any new strings were ready for translation (p4TIVU-5Jq-p2)?
• For changes affecting Jetpack: Have we added the "[Status] Needs Privacy Updates" label if this pull request changes what data or activity we track or use (p4TIVU-aUh-p2)?

[github-actions]

Calypso Live (direct link)

https://calypso.live?image=registry.a8c.com/calypso/app:build-125573

Jetpack Cloud live (direct link)

https://calypso.live?image=registry.a8c.com/calypso/app:build-125573&env=jetpack

Automattic for Agencies live (direct link)

https://calypso.live?image=registry.a8c.com/calypso/app:build-125573&env=a8c-for-agencies

[matticbot]

This PR modifies the release build for the following Calypso Apps:

For info about this notification, see here: PCYsg-OT6-p2

• notifications
• wpcom-block-editor

To test WordPress.com changes, run install-plugin.sh $pluginSlug update/plans-grid-header-price-cleanup on your sandbox.

[matticbot]

Here is how your PR affects size of JS and CSS bundles shipped to the user's browser:

Sections (~112 bytes added 📈 [gzipped])

name                  parsed_size           gzip_size
update-design-flow         +200 B  (+0.0%)     +112 B  (+0.0%)
plugins                    +200 B  (+0.0%)     +112 B  (+0.0%)
plans                      +200 B  (+0.0%)     +112 B  (+0.0%)
link-in-bio-tld-flow       +200 B  (+0.0%)     +112 B  (+0.0%)
jetpack-app                +200 B  (+0.0%)     +112 B  (+0.1%)

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 (~112 bytes added 📈 [gzipped])

name                                             parsed_size           gzip_size
async-load-signup-steps-plans-theme-preselected       +200 B  (+0.1%)     +112 B  (+0.1%)
async-load-signup-steps-plans                         +200 B  (+0.1%)     +112 B  (+0.1%)

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.
Gzip Size: Compressed size of the JS and CSS files. This much data needs to be downloaded over network.

Generated by performance advisor bot at iscalypsofastyet.com.

[chriskmnds]

@jeyip still WIP and something to ship last. Let me know if you get a chance to review the code at least. The functionality should be the same. The changes circle around how we align the header price for a non-discounted plan in a grid with discounts e.g. the "Free" plan's price when term-savings are enabled or intro offers (like first-year currency) for the other plans.

[jeyip]

Thanks for tagging. I'll check this out, test, and review first thing tomorrow morning 👍

[jeyip]

Testing

Requirements

For each of the flows listed below:

• Ensure for yearly and above the savings are displayed as per media above
• Can still purchase multi-year plans, annual plans, and monthly plans
• Try the same with a coupon ( Ex. federate 25 ) or active intro offer (need to mock/code it) and ensure those discounts render instead for the respective plans in the features + comparison grid
  • Used coupon federate25 which applies only to annual business and ecommerce plans
  • Visited url http://calypso.localhost:3000/start/plans?flags=plans%2Fterm-savings-price-display&coupon=federate25

Features Grid

Comparison Grid

Flows:

• /start
• /plans

Browsers

• Chrome

[jeyip]

Updated the PR description with some testing instructions for posterity. Feel free to edit anything if it looks off 🙂

[jeyip]

Thanks for outlining the evolution of the PR in distinct commits. Super helpful

There's something that I'm not quite clear on -- would you mind explaining a bit more about the change between wrap header discounting logic in a hook and move it to header-price context?

I think I'm missing something, because it seems like we could avoid the extra file and header price context provider by keeping the const [ isAnyVisibleGridPlanDiscounted, setIsAnyVisibleGridPlanDiscounted ] = useState( false ); hook in the header-price component. Is it because we're going to reuse the logic outside of the component in the near future?

[chriskmnds]

@jeyip thanks for reviewing!

There's something that I'm not quite clear on -- would you mind explaining a bit more about the change between wrap header discounting logic in a hook and move it to header-price context?

One of the points was to eliminate side effects/knowledge stemming from the component traversing all the grid plans to determine if another header price is discounted. The hook method merely centralised the logic inside an internal hook, but it still needed all the extra knowledge to determine if something else (another instance of the component) has been discounted. With context (or the external store) the component simply "announces" that it has been discounted, and the rest can deal with that. That's the essence pretty much. It also reduces all the traversals happening on rerenders.

I think I'm missing something, because it seems like we could avoid the extra file and header price context provider by keeping the const [ isAnyVisibleGridPlanDiscounted, setIsAnyVisibleGridPlanDiscounted ] = useState( false ); hook in the header-price component. Is it because we're going to reuse the logic outside of the component in the near future?

I think what you are missing is the component being rendered in a list. A local variable will be local to each instance.

[jeyip]

I think what you are missing is the component being rendered in a list. A local variable will be local to each instance.

Ah yes that's it. Thanks for elaborating 👍

One of the points was to eliminate side effects/knowledge stemming from the component traversing all the grid plans to determine if another header price is discounted. The hook method merely centralised the logic inside an internal hook, but it still needed all the extra knowledge to determine if something else (another instance of the component) has been discounted. With context (or the external store) the component simply "announces" that it has been discounted, and the rest can deal with that. That's the essence pretty much. It also reduces all the traversals happening on rerenders.

Going over this before I sign off tonight... 👀

Edit: Ahhh yeah I'm following. Thanks for explaining 🙂

[jeyip]

There are some failing unit tests, but judging by the failures, they're simple reference errors. Things lgtm after those are addressed 🙂