Plans Grid Refactor: Move action override related props to the relevant data structure

[ddc22]

• Related to: Plans 2023: Revise and split the grid component props #82335

Proposed Changes

• To avoid prop drilling the following props were were changed to be derived from plan action override.
  • canUserManageCurrentPlan?: boolean | null;
  • currentPlanManageHref?: string;
• This reduces unnecessary prop drilling across the tree.

Testing Instructions

Case 1

• Purchase a plan in your own account
• Go to /plans page and make sure on the plan that is activated for the given site the button reads manage plan

Case 2

• Add yourself to another site (with a paid plan) as a contributor
• Make sure only view plan is visible and not manage plan

Pre-merge Checklist

• Has the general commit checklist been followed? (PCYsg-hS-p2)
• https://wpcalypso.wordpress.com/devdocs/docs/testing/index.md 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-ajp-p2)?

[github-actions]

Calypso Live (direct link)

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

Jetpack Cloud live (direct link)

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

[matticbot]

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

Sections (~79 bytes removed 📉 [gzipped])

name                  parsed_size           gzip_size
update-design-flow         -730 B  (-0.1%)      -79 B  (-0.0%)
plugins                    -730 B  (-0.0%)      -79 B  (-0.0%)
plans                      -730 B  (-0.1%)      -79 B  (-0.0%)
link-in-bio-tld-flow       -730 B  (-0.0%)      -79 B  (-0.0%)
jetpack-app                -730 B  (-0.2%)      -79 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 (~79 bytes removed 📉 [gzipped])

name                                             parsed_size           gzip_size
async-load-signup-steps-plans-theme-preselected       -730 B  (-0.1%)      -79 B  (-0.0%)
async-load-signup-steps-plans                         -730 B  (-0.1%)      -79 B  (-0.0%)

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]

Thanks for thinking about this. prop-drilling is experience-killing :D

Something doesn't feel right with these sitting here though (at the top level at least). PlanActionOverrides was meant to have a clear semantic, holding a set of props for overriding the CTAs on the page (not a placeholder for relevant helpers). It might not be a super polished approach though 🤷‍♂️

For action overrides I'd expect a description like the following:

Used for overwriting the action button properties for a respective context and plan e.g. loggedInFreePlan used to customise the free-plan CTA in a logged-in/user session. An action override contains a callback (the event handler to override), text (the text to display), status (not sure how to explain this exactly)... etc.

Before we delve any further, I have a quick idea that might resonate: How about we create a currentPlan override, and pass the relevant data (href and text) instead? so we won't need to pass canUserManageCurrentPlan and currentPlanManageHref into the grid at all. if it makes sense? 🤔

[ddc22]

Something doesn't feel right with these sitting here though (at the top level at least)

Thanks @chriskmnds I wanted to spark some conversation here, had the same feeling myself.

How about we create a currentPlan override

This is perfect! 😁

[chriskmnds]

Thanks for working out these changes!

My only concern here is planActionOverrides?.currentPlan feels like something of a "required" property, otherwise we'd still be rendering a button but with no text. That will probably never be the case technically, but semantically/conceptually that is the case.

I think the above is fine, especially if we want to keep the two texts living close to each other. but let's maybe make it more explicit that this is what we are doing. I would maybe create a separate "default" case (the button with no functionality) and conditionally render it if no planActionOverrides?.currentPlan exists. That way it's a little more clear that we are "replacing" something (instead of "injecting" things into it).

I will explore a little later and send a commit if I come up with something :-)

[ddc22]

@chriskmnds I have added a default case! Can you have another look?

91f88ec

[matticbot]

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

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

• blaze-dashboard
• odyssey-stats

To test WordPress.com changes, run install-plugin.sh $pluginSlug fix/move-action-override-related-props on your sandbox.

[chriskmnds]

I tested the first case and it works like in production. I had a bit of trouble testing the second case. I end up with "you are not authorised to view this page" as a contributor. That's also the case in production. If you verified that it works, then it's good for me.

There does seem to be an issue for me regarding first case (that also happens in production) - I think related to storage add-ons (similar/same to what I mentioned in @jeyip 's PR #84289 (comment)):

Screen.Recording.2023-11-24.at.12.09.17.PM.mov

Code-wise looks great. Thanks for following up. I left a comment in case it triggers any ideas for further refactors.

[chriskmnds]

Thanks for doing this refactor.

NIT / or to think about for later (if it makes sense): I wonder if a "general default state" would work better instead. So all CTAs to have same default state ("Contact support" perhaps) that is overriden either by the planActionOverrides or internally in actions as done already (so "Upgrade" or "Upgrade to ..", etc) are all replacing "Contact support". That may be the case already if we chase the code, but I think it could be more clear maybe.

[ddc22]

Yes ideally this would make the code much simpler. The only problem is with testing all the related variants. The action buttons are a giant bowl of spaghetti where changes have been done for various use cases which are technically not the concern of the plans grid.

Once we complete the plans grid packaging work it would be a good immediate follow up to audit all use cases. Cleanup the money dead case (I have a hunch there a few) and possibly force the client to inject the related text and possible styles and callback related to the button.

However I will see if a more clear "default" mechanism could be set in place to be overridden subsequently in a followup.