The selected plans state on the drop downs should not `reset` when there is a parent state refresh

[ddc22]

• Fixes Automattic/martech#2554

Proposed Changes

The ComparisonGrid component now includes a "Compact Display" configuration tailored for smaller screens, streamlining plan feature comparisons. End-users can easily customize the order of plans to suit their preferences within the grid. To maintain order consistency, especially during unmounts triggered by plan interval changes, this state is now efficiently managed within the WpcomPlansUI store.

When a change in intervalType propagates down to the ComparisonGrid as a prop, the implemented solution in this PR detects this prop change. It then seamlessly transforms the current plan order into a set that aligns with the currently selected plan interval. This transformation is designed to avoid any glitches or flashes during the transition.

https://github.com/Automattic/wp-calypso/assets/3422709/0f69dabf-e91a-48de-a5d0-3c3d84977eb5

Testing Instructions

• On the comparison grid, on smaller resolutions, make sure the selected dropdown order does not change on interval change

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-ajp-p2)?

[github-actions[bot]]

Calypso Live (direct link)

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

Jetpack Cloud live (direct link)

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

Automattic for Agencies live (direct link)

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

[matticbot]

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

Sections (~43 bytes added 📈 [gzipped])

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

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

name                                             parsed_size           gzip_size
async-load-signup-steps-plans-theme-preselected       +141 B  (+0.0%)      +43 B  (+0.0%)
async-load-signup-steps-plans                         +141 B  (+0.0%)      +43 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.

[ddc22]

@chriskmnds I have not based these changes off of @aneeshd16's change :) I did some clarification of the Compact View . Happy to do any changes as required. This change is now functional and ready to review. Please do have a look when you have some time.

[ddc22]

Thanks for these comments @chriskmnds I am working through them.

and if there are any concerns with storing the actual GridPlans there, then I'd still like us to use this store slice as the index of plans visible in the grid (so store visiblePlans in that case, as PlansSlug[]). then still access that in the components in place of visibleGridPlans, and derive the GridPlan from GridPlansIndex in that case

For now I am going to store visiblePlanSlugs instead of the plans themselves, Because this means we would have to move the types to the store as well to avoid circular import. Pushing the visible grid plans could possibly done as a follow up together with moving all the related types to the store as well.

[ddc22]

Honestly, this feels complex to me. There is a lot going on here, which I'd prefer we revise the approach at a higher level first, before diving into the details.

I want to explain the reason for the complicated hook in case it is not clear so that we can work to reduce complexity from there.

The problem the complicated hook solves can be observed by relying completely on the store state as shown with the change shown below. This change ignores the implementation of the hook and forces the visibleGridPlans variable to rely only on the store state.

With the above change if the interval is switched there will be glitch when switching intervals.

https://github.com/Automattic/wp-calypso/assets/3422709/44f13902-74ed-4de1-b7ad-f7adaf040fc1

[chriskmnds]

With the above change if the interval is switched there will be glitch when switching intervals.

See the patch that fixed that glitch previously (it existed before): https://github.com/Automattic/wp-calypso/pull/82994

^ in case it informs what we are doing/skipping here

I haven't looked at anything else, just remembered that we patched something about that before. Thanks for addressing the feedback though, as always. will take a look next week

[ddc22]

Thanks for the feedback Christos, I have addressed all of them :)

To summarise:

1 - The single source of truth is now hook state all store related code is cleaned up. I incorrectly assumed that hook state will be lost on unmount, gained a deap learning here although it's kind of obvious. 2 - The hook now provides a setter setVisiblePlans which is being used when the grid is slized based on the viewPort width as well as selections change. 3 - For transforming plans when the interval changes, I have introduced planCompare util inspired by planMatches. Curious to know your thoughts about the approach.

Beyond this, looking forward to explore any other avenues to make that hook simpler if possible!

[ddc22]

Thanks for the comments @chriskmnds I am going to close this PR now since I have to shift to other priorities and the code from this is mostly unnecessary. Will use the learnings from this for a future PR if I find time :).

[chriskmnds]

Thanks for the comments @chriskmnds I am going to close this PR now since I have to shift to other priorities and the code from this is mostly unnecessary. Will use the learnings from this for a future PR if I find time :).

Oh please don't close the PR (unless you want to split it out better). It's very close - I was hoping for a bit more tuning and we can ship soon. There are also potential refactors that can fit on top of this.

If you've run out of time, then I'll take over here. So no need to close this at all if you don't intend to push it out.

[ddc22]

@chriskmnds I was going to close it because when I come back I might have to most probably face merge hell 😁 . Far easier to just create a new PR 👍 and just copy paste the learnings from here! If you don't have a strong preference to ship this soon, I would like to pick this up back myself :) [Unless I am truly without time to look into it in the coming weeks]

[ddc22]

@chriskmnds turns out I do have enough time so I did the changes that you requested. Please do have a look when you have some time.

[ddc22]

@chriskmnds I am planning to merge this. Let me know if you have any objections.

[southp]

@ddc22 I'm moving it to Waiting to Ship since all the feedback convos seem to have resolved. When you have a moment, let's deploy it after resolving the merge conflicts :)

[matticbot]

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

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

• blaze-dashboard
• command-palette-wp-admin
• odyssey-stats
• wpcom-block-editor

To test WordPress.com changes, run install-plugin.sh $pluginSlug fix/maintain-selected-plan-when-switching-intervals on your sandbox.