-
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
Merged
Merged
Changes from all commits
Commits
Show all changes
57 commits
Select commit
Hold shift + click to select a range
5a85096
Add storage upgrade dropdown behind feature flag
jeyip 267171f
Update default selected value to empty string
jeyip c6f8f58
Create plans add ons list
jeyip c8ee9dc
Remove references to feature in add ons list
jeyip dcf8c14
Add 100gb and 200gb constants
jeyip 20e46e2
Isolate storage add ons from storage features
jeyip d531bb2
Update constants
jeyip 0315d30
Update inline comment
jeyip 7178410
Add feature flag to dropdowns
jeyip d1feb85
Refine storage option and storage feature naming
jeyip cd335f2
Fix storageFeatures type
jeyip 93096a6
Fix typescript errors
jeyip 685cc91
Remove unnecessary AddOns type
jeyip 07d4adf
Update inline comment
jeyip 0f86021
Move add ons list into calypso products
jeyip 9fdef31
Export add ons list from calypso products
jeyip f63d006
Pass feature flag as props through plans features main
jeyip 6db6897
Update OnboardingPricingGrid2023Props type
jeyip 778e27a
Remove unnecessary selected storage initialization
jeyip f2c2900
Switch dropdown to a component from @wordpress/component
jeyip db287e3
Remove unnecessary storageFeature check
jeyip da951d9
Fix dropdown alignment
jeyip 1fcd3bf
Initialize state as a class member
jeyip 4d7f798
Explicitly type state initialization
jeyip dafb61b
Convert storage add on dropdown to functional component
jeyip dfdf23b
Fix add on dropdown typescript errors
jeyip 02e9020
Fix features grid props typescript error
jeyip 1e78d5b
Refine selected storage type
jeyip 9463235
Update storageOptions transform
jeyip af8a1ca
Fix storage label bug
jeyip 1fbb2aa
Fix dropdown label color
jeyip ae34c26
Add inline comment
jeyip c8932c9
Fix select control change handler typescript error
jeyip a9e8a4d
Remove unnecessary inline comment
jeyip c364c93
Update storage title condition
jeyip b501a7a
Move all storage addons into features list
jeyip 3e75322
Remove add ons list export
jeyip c9a86ef
Remove unnecessary addons export
jeyip 61ecf55
Update WPComStorageAddOnSlug type
jeyip b125058
Fix typescript errors
jeyip a699304
Update wpcom storage add on type
jeyip 756a19a
Update to add-on string
jeyip 4255382
Add inline comment for showUpgradeableStorage
jeyip f228e42
Replace getFeatureByKey method
jeyip fafa524
Use slug instead of key
jeyip 301cce1
Add feature object to storageOptions
jeyip b471ec0
Fix typescript errors
jeyip 3adb0f7
Fix storage rendering bug
jeyip f1cd050
Create storageOption exportable type
jeyip 7883c73
Update feature object add on slugs and labels
jeyip 8f4a237
Generate storage add on label in presentation layer
jeyip ba03159
Switch to isAddOn property
jeyip 5c8ae30
Determine isAddOn property in get2023PricingGridSignupStorageOptions
jeyip 847acb4
Add inline comment describing isAddOn property
jeyip e23a589
Remove unused featureObject isAddOn property
jeyip e836e80
Fix comparison grid strings
jeyip aa00ccc
Remove unnecessary components prop
jeyip File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
57 changes: 57 additions & 0 deletions
57
client/my-sites/plan-features-2023-grid/components/storage-add-on-dropdown.tsx
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,57 @@ | ||
| import { CustomSelectControl } from '@wordpress/components'; | ||
| import { TranslateResult, useTranslate } from 'i18n-calypso'; | ||
| import { PlanSelectedStorage } from '..'; | ||
| import { PlanProperties } from '../types'; | ||
| import { getStorageStringFromFeature } from '../util'; | ||
|
|
||
| type StorageAddOnDropdownProps = { | ||
| planProperties: PlanProperties; | ||
| selectedStorage: PlanSelectedStorage; | ||
| setSelectedStorage: ( selectedStorage: PlanSelectedStorage ) => void; | ||
| }; | ||
|
|
||
| export const StorageAddOnDropdown = ( { | ||
| planProperties, | ||
| selectedStorage, | ||
| setSelectedStorage, | ||
| }: StorageAddOnDropdownProps ) => { | ||
| const { planName, storageOptions } = planProperties; | ||
| const translate = useTranslate(); | ||
|
|
||
| // TODO: Consider transforming storageOptions outside of this component | ||
| const selectControlOptions = storageOptions.reduce( | ||
| ( acc: { key: string; name: TranslateResult }[], storageOption ) => { | ||
| const title = getStorageStringFromFeature( storageOption.slug ); | ||
| if ( title ) { | ||
| acc.push( { | ||
| key: storageOption?.slug, | ||
| name: title, | ||
| } ); | ||
| } | ||
|
|
||
| return acc; | ||
| }, | ||
| [] | ||
| ); | ||
|
|
||
| const defaultStorageOption = storageOptions.find( ( storageOption ) => ! storageOption?.isAddOn ); | ||
| const selectedOptionKey = selectedStorage[ planName ] || defaultStorageOption?.slug || ''; | ||
| const selectedOption = { | ||
| key: selectedOptionKey, | ||
| name: getStorageStringFromFeature( selectedOptionKey ), | ||
| }; | ||
| return ( | ||
| <CustomSelectControl | ||
| label={ translate( 'Storage' ) } | ||
| options={ selectControlOptions } | ||
| value={ selectedOption } | ||
| onChange={ ( { selectedItem }: { selectedItem: { key?: string } } ) => { | ||
| const updatedSelectedStorage = { | ||
| [ planName ]: selectedItem?.key || '', | ||
| } as PlanSelectedStorage; | ||
|
|
||
| setSelectedStorage( updatedSelectedStorage ); | ||
| } } | ||
| /> | ||
| ); | ||
| }; |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
To top it up, I would also question the need for
FEATURE_50GB_STORAGE_ADD_ONas a separate object here. It's just a repeated structure toFEATURE_50GB_STORAGEfor the most part with a different slug right? Obviously, alternative (to use same structure) needs a little deeper consideration.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.
The distinct difference will be the inclusion of
getQuantityon the feature object. and the label strings. https://github.com/Automattic/wp-calypso/pull/79041/files/afe13155242a279e041c346515508fe2383fd22a#r1279555091In my head, it's natural to have them as two distinct objects. One is functionality to be purchased and another represents functionality inherent to a plan.
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.
Hmm gotcha. Potentially separate things. Although, both could be described in terms of
x * unitright? 🤔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.
We could 👍 My decision to separate was more of an intuition -- it seems natural to see add ons and features as two different concepts, and I wouldn't be surprised if there's more divergent attributes in the future. As of right now though, they both totally could be described in terms of x * unit.
If we feel strongly about joining the two, I can make that happen.
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.
I don't feel too strongly. On one end, I feel a separate object allows us to "do more". On the other end, I feel strongly about NOT replicating anything, and YAGNI (are we actually going to have any unique functionality for these "add-on" features)?
Let's maybe confirm whether we have standard features being used as add-ons already. I think I'm seeing a few in
my-sites/add-ons/hooks/use-add-ons. If that's the case, then we have a mixture of things happening already:add_onand for the most part replicate the original "non add-on" feature to introduce other wording, properties, etc.add_onprefix, probably no alternative titles).Make the best judgment. with @southp pushing for this to move forward, I'll retract from this conversation.
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.
I haven't followed the conversation here too closely, but just chiming in regarding the feature check.
Yes, the existing 50, 100, etc. are the ones used in bundles -- and are individual products mapped to a feature. They have no quantity.
As the features system isn't aware of quantity, the feature that's mapped to this new product is "Space 1 GB" (signaling that the subscription has this product), and the packages are just "stacks" of 50 or 100 of this product.
We keep track of quantity on subscription level, but there's no easy way to do it on feature level (e.g. by mapping a certain quantity to a certain feature), as the feature checks are boolean -- either the subscription has it, or it doesn't. :)
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 chiming in @gmovr .
I think we are just complicating things. Somehow this FeaturesList doesn't feel like a clearly defined structure. Is an add-on a feature? Does it ever map to the plan features? Would it make sense to return that from an endpoint
/plan-features/[plan_id]? Are features meant to be something else than what plans or other products are bundled with?My guess the answer to all of the above is "no". and if so, then "add-on" has no place in this list. Separate list, fine. Separate hook, even better (and looks like we have one already from the "add-ons" work).
and maybe no clear answers to any of the above - it might not be
yes/no, maybe a "depends on how you look at it" approach (and how should we look at it then?)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 exploring this concept in follow-up PRs and pinpointg core issues from there @jeyip , and thanks for further clarifcation, @chriskmnds :)
To summarize, the baseline thing we need to see are:
FEATURE_XXGB_STORAGE_ADD_ONneeds to be gone.If that's correct, then unfortunately the exploratory PR #80114 is not in the right direction since that unwanted feature slug is still there. In that case, I think it would be the best if we can create a focal issue/PR solely for sorting this out –– the exploratory PR seems also to want to implement the dropdown functionality, but I feel it would add too much other concerns and deviate from our original purpose of shipping this PR first. I might be wrong, though :)
@jeyip Could you help create an issue for this, add it to the epic issue, and priortize it as the topmost from here? I'd be grateful :)
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.
I think we are aligned on this. What @southp described and the exploration in #80114 speak of the same concerns to me.
Maybe we could have defined a separate structure or hook for add-ons here and avoid conflating with the features list. Then a follow-up could be to bridge that structure with the existing
use-add-ons(which would be a bigger undertaking).Speaking of which - @jeyip - I don't think plan ui package (
plans-features-2023-grid) should host that hook. As a pointer for #80114 , I would suggest a separateadd-onspackage in the monorepo and export the hook from there. Maybe data-stores, but separate one can also exist to migrate rest of add-ons section there (I wonder when that will be put on pedestal for reuse). Definitely not under plans ui package though IMO. But lets this convo there / #80114There 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 chiming in y'all!
Of course -- I'll take care of this on Monday