-
Notifications
You must be signed in to change notification settings - Fork 2k
Plans 2023: Split and export the individual grids (FeaturesGrid, ComparisonGrid) - take 2 #82249
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
Conversation
Here is how your PR affects size of JS and CSS bundles shipped to the user's browser: Sections (~52 bytes removed 📉 [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 (~52 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. |
2156557
to
213a9e4
Compare
/* | ||
* TODO clk: On PlansGridProps: | ||
* 1. These props need to be split into what's needed separately for the internal grids and what's needed for the exported wrappers. | ||
* This is currently also used as the internal type for the FeaturesGrid (wrongly). | ||
* - onUpgradeClick is only needed for the wrappers exported from here. Internally the grids take a transformed | ||
* handleUpgradeClick/onUpgradeClick function (force renamed to handleUpgradeClick in FeaturesGrid) | ||
* - allFeaturesList is only relevant for the ComparisonGrid | ||
* - gridPlanForSpotlight is only relevant for the FeaturesGrid | ||
* - etc. | ||
* 2. Explicitly set optional props and default values. Only absolutely vital props should be required. | ||
*/ |
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 we ought to address in a bit of immediate follow-up. We may consider splitting these props across several pieces, and updating the respective READMEs while at it, possibly:
ContextProps
(not exported) all the props that pertain to things added to the contextSharedPlansGridProps
(not exported) all the props that are shared between the FeaturesGrid and ComparisonGrid (et al)ComparisonGridProps
FeaturesGridProps
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.
Created a separate issue for this #82335
Performance optimisations addressed in #82403 |
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.
Following the test plan I previously wrote it all works as expected :) On top of that test plan, I've also tried out the storage add-on dropdown, term toggle, plan picker of the comparison grid, and the mobile resolutions. Oh my, we really should compile a QA guide or something 🙃
I do notice there are some things broken in the comparison grid in /plans
, e.g. the styling of the Free plan is missing, the tooltips are flashing, "show features" requires 3 clicks to be expanded. However, those also happen in production so should be tracked and resolved separately from this 🚢
Thanks @southp .
I don't notice these in the live image 🤔 Do you mind confirming and we can create a few issues for them?
This sounds pretty severe. Do you mean the "show all features" toggle in the mobile cards? |
Related to #78266
Takes over #80915
Proposed Changes
Converts the main package component
plans-grid
into an index of individual parts, exported and assembled at the consuming end. We effectively now exportFeaturesGrid
andComparisonGrid
separately, and a singlegridPlans
prop in place ofgridPlansForFeauresGrid
, etc.Additional Notes
ComparisonGrid
carries its title along, which is something incredibly small that we can handle in a follow-up.Testing Instructions
/start
and/plans/[ paid | free ]
render as beforePre-merge Checklist