-
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
Storage Add Ons: Move UI state into wpcom-plans-ui data store #80158
Conversation
@@ -14,8 +15,19 @@ const showDomainUpsellDialog: Reducer< boolean | undefined, WpcomPlansUIAction > | |||
return state; | |||
}; | |||
|
|||
const selectedStorageAddOnsForPlans: Reducer< |
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.
Note:
Naming this slice of state felt odd because I wasn't sure how to interpret the existing convention. A sibling reducer is called showDomainUpsellDialog. In the end, I tried to make the name as specific as possible, but I'm totally open to changing this
5f534c2
to
bc451bd
Compare
@chriskmnds would you mind taking a look at this? |
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.
Thanks for working on this! Left some comments below, #80158 (comment) seems critical and merits investigation. Overall looks good to me though.
const reducer = combineReducers( { | ||
showDomainUpsellDialog, | ||
selectedStorageAddOnsForPlans, |
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.
Asking here, so we can resolve in a thread (so not exactly related to this piece of code).
Do we expect to retrieve the selected storage from a location outside of the plans grid package (plan-features-2023-grid
)? e.g. from checkout
maybe? If that's the case, then we will need to set the value to persist
in the data store, and possibly call an action to reset it when rendering the grid components (features table or comparison grid) in a "fire once" hook.
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.
Do we expect to retrieve the selected storage from a location outside of the plans grid package (plan-features-2023-grid)? e.g. from checkout maybe?
Not for the time being. checkout
already has their own state management in place for add ons
TestingFor ✔️ The dropdown works as expected both with keyboard and mouse. The selection is persisted. • 50 GB:
• 100 GB:
• 150 GB:
|
NVM, I realized I was supposed to go to |
Thanks for all the feedback y'all! Taking a look now 👍 |
bc451bd
to
3be404b
Compare
6aeef7c
to
add4f03
Compare
Here is how your PR affects size of JS and CSS bundles shipped to the user's browser: Sections (~2040 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 (~2169 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. |
36d7e45
to
09e9f0c
Compare
State doesn't initialize as undefined -- it's the substate selectedStorageAddOnsForPlans that does. We update the optional chaining of the getStorageAddOnForPlan selector to reflect this
309aa01
to
b2fcca4
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.
Code looks good and the PR tests well after the recent changes. 👍
To avoid any more merge conflicts with this PR I'll be merging it shortly. All comments have been addressed, but @chriskmnds if there's any more you'd like to add when you're back from AFK, I'll be happy to draft immediate follow-ups for any new concerns that pop up. |
Follow-up to #79041 (comment)
Proposed Changes
As part of the plans 2023 pricing page refactor, @chriskmnds is breaking apart the pricing grid components ( features grid & comparison grid ) and exporting them individually.
To prevent roadblocks to his work, we want to avoid sharing UI state locally between the features grid and the comparison grid. As a result, we're moving any storage add ons UI state away from local state management and into the
wpcom-plan-ui
data store.GIF
Testing Instructions
/start/plans?flags=plans/upgradeable-storage
Pre-merge Checklist