-
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
Plans 2023: Implement storage add on dropdown #79041
Conversation
Notes: Hoping to have a PR or two that satisfies all requirements outlined here for Plans 2023 onboarding pages sometime tomorrow 🤞 Need to work on:
|
This PR modifies the release build for editing-toolkit To test your changes on WordPress.com, run To deploy your changes after merging, see the documentation: PCYsg-mMA-p2 |
This PR modifies the release build for happy-blocks To test your changes on WordPress.com, run To deploy your changes after merging, see the documentation: PCYsg-r7r-p2 |
Here is how your PR affects size of JS and CSS bundles shipped to the user's browser: App Entrypoints (~222 bytes added 📈 [gzipped])
Common code that is always downloaded and parsed every time the app is loaded, no matter which route is used. Sections (~589 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 (~10548 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. |
a38dc55
to
9caaedc
Compare
@chriskmnds Addressed the first wave of comments 😄 This PR could use another look. I'll touch base with designers about the styling deviations with what was in the mocks and with the @wordpress/components dropdown we're going with when the PR is looking more finalized. |
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-wise looks great. The dropdown will probably need some restyling, but obviously something to go through designer input. If we keep using the Core component, then maybe the restyled one can go straight into @automattic/components
, and let that propagate in the app and gradually deprecate the local/unpackaged Calypso component. Not sure if we are just adding more complexity though by keeping both and not just migrating the existing Calypso one under @automattic/components
🤷♂️
I tested in start
and plans
as outlined. A few points I noticed:
- Without feature flag, title/"Storage" missing in respective columns:
- With feature flag, "Storage"/title over dropdown should probably be bold like the rest:
- I don't see any change in price in the UI when selecting different storage, or additional product added to cart when going to checkout. Is this something to come after backend changes?
01ddff4
to
aa00ccc
Compare
This Pull Request is now available for translation here: https://translate.wordpress.com/deliverables/8679268 Thank you @jeyip for including a screenshot in the description! This is really helpful for our translators. |
slug: string; | ||
// Determines if the storage option is an add-on that can be purchased. There are a mixture of patterns | ||
// to identify add-ons for now, and we're temporarily adding one more | ||
isAddOn: boolean; |
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.
could this be set to optional and avoid all the isAddOn: false
assertions above (that make up most cases)
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.
Yes! I'll make a note of this and take care of it in either an upcoming or separate PR
Translation for this Pull Request has now been finished. |
Related to https://github.com/Automattic/martech/issues/1907
Proposed Changes
User should be able to select additional storage for Business and Ecommerce plans:
What will be implemented in follow-up PRs:
GIF
Onboarding
/plans
Testing Instructions
"plans/upgradeable-storage"
inconfig/development.json
and restarting the dev environmentPre-merge Checklist