Merge single product backup body component master #14056
Conversation
…ux and rest API calls
Co-Authored-By: Marin Atanasov <8436925+tyxla@users.noreply.github.com>
…iner to single-product-backup__header-price
Co-Authored-By: Marin Atanasov <8436925+tyxla@users.noreply.github.com>
Co-Authored-By: Marin Atanasov <8436925+tyxla@users.noreply.github.com>
Co-Authored-By: Marin Atanasov <8436925+tyxla@users.noreply.github.com>
This is an automated check which relies on |
gravityrail
added
[Status] Needs Review
| planPrice, | ||
| } ) { | ||
| export function PlanPriceDisplay( { backupPlanPrices, currencySymbol } ) { | ||
| const dailyBackupYearlyPrice = backupPlanPrices.jetpack_backup_daily.yearly; |
There was a problem hiding this comment.
Do we need any sort of property existence guards here?
There was a problem hiding this comment.
In this case I don't think so. This is really just a helper component for the header component that comes in the final PR, where the property is guaranteed to exist. I prefer checking for existence much closer to the Redux connection so that helper components like this can be kept free of prop validation to keep them as simple as possible.
There was a problem hiding this comment.
the property is guaranteed to exist
These are famous last words 😉
| { | ||
| type: 'daily', | ||
| name: __( 'Daily Backups' ), | ||
| price: backupPlanPrices.jetpack_backup_daily.yearly, |
There was a problem hiding this comment.
It feels odd that we're passing an entire backupPlanPrices object to this component and only using pieces of it like this. Are we passing more than we need here?
| static propTypes = { | ||
| backupPlanPrices: PropTypes.object, | ||
| currencySymbol: PropTypes.string, | ||
| setSelectedBackupType: PropTypes.func, |
| daily: __( 'Upgrade to Daily Backups' ), | ||
| }; | ||
|
|
||
| const backupOptions = [ |
There was a problem hiding this comment.
Feels odd to be defining this in every re-render.
| @@ -36,3 +81,85 @@ export function PlanRadioButton( { | |||
| </label> | |||
| ); | |||
| } | |||
|
|
|||
| // eslint-disable-next-line no-unused-vars | |||
| class SingleProductBackupBody extends React.Component { | |||
There was a problem hiding this comment.
Feels odd that we have so many components in this same file. I'd suggest keeping one component per file for consistency and readability.
| render() { | ||
| const { currencySymbol, backupPlanPrices, selectedBackupType, upgradeLinks } = this.props; | ||
|
|
||
| const upgradeTitles = { |
There was a problem hiding this comment.
Feels odd to be defining this and upgradeTitles in every re-render.
| radioValue, | ||
| planPrice, | ||
| } ) { | ||
| export function PlanPriceDisplay( { backupPlanPrices, currencySymbol } ) { |
There was a problem hiding this comment.
Seems like this component name is pretty generic but functionality-wise it's pretty backup-specific. Should we rename? Or should we allow for products to be specified and keep this as a simple presentational component?
There was a problem hiding this comment.
I'd rather make it specific, as I did not design this with it being generic in mind. We can always make it generic later if we need to.
There was a problem hiding this comment.
Sounds good to me 👍 Happy to change this name to a more specific one.
jeherve
added
[Status] Ready to Merge
😅 I'm not even sure if I left this feedback in the right place, but please let me know if I didn't. |
|
@tyxla @robertf4 @jeherve I strongly recommend at this point that we get this merged and then open a separate PR for @tyxla's fixes. I realise that is not ideal, but the testing and feedback loop on these changes has gotten way too complicated and I think the greater good here is to get them merged, integrated and refined all together. |
I will create issues for the feedback here and get this merged, then rebase #14018 and get that merged. |
|
Created #14065 to keep track of feedback |
robertf4
added
[Status] Ready to Merge
matticbot
added
[Status] Needs Changelog
and removed
[Status] Ready to Merge
|
I think this PR introduced some unrelated |
Replacement for #14027 which targets master
Changes proposed in this Pull Request:
Is this a new feature or does it add/remove features to an existing part of Jetpack?
See the P2 here for the design of this PR: p1HpG7-7MK-p2 (specifically the part titled "WP Admin Desktop")
See the P2 here for the overall MT: p1HpG7-7ET-p2
Testing instructions:
Proposed changelog entry for your changes: