Skip to content
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

Add SingleProductBackupBody Component #14027

Conversation

robertf4
Copy link
Contributor

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:

  • No changelog entry needed.

@robertf4 robertf4 requested a review from a team November 14, 2019 06:13
@robertf4 robertf4 self-assigned this Nov 14, 2019
@robertf4 robertf4 added [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. Admin Page React-powered dashboard under the Jetpack menu Plans and removed [Status] In Progress labels Nov 14, 2019
Copy link
Contributor

@delawski delawski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@robertf4 This looks good in general. I left some minor comments, please have a look.

_inc/client/plans/single-product-backup.jsx Outdated Show resolved Hide resolved
_inc/client/plans/single-product-backup.scss Outdated Show resolved Hide resolved
_inc/client/plans/single-product-backup.scss Outdated Show resolved Hide resolved
_inc/client/plans/single-product-backup.jsx Outdated Show resolved Hide resolved
_inc/client/plans/single-product-backup.scss Outdated Show resolved Hide resolved
@delawski delawski added [Status] Needs Author Reply We would need you to make some changes or provide some more details about your PR. Thank you! and removed [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. labels Nov 14, 2019
@jeherve jeherve added this to the 8.0 milestone Nov 14, 2019
_inc/client/plans/single-product-backup.jsx Outdated Show resolved Hide resolved
_inc/client/plans/single-product-backup.jsx Outdated Show resolved Hide resolved
_inc/client/plans/single-product-backup.jsx Outdated Show resolved Hide resolved
_inc/client/plans/single-product-backup.jsx Show resolved Hide resolved
@robertf4 robertf4 force-pushed the add/plan-price-display-component branch from ae49ac7 to d2e8664 Compare November 15, 2019 03:13
@robertf4 robertf4 force-pushed the add/single-product-backup-body-component branch from 00c9da8 to fe15730 Compare November 15, 2019 04:16
@robertf4 robertf4 force-pushed the add/plan-price-display-component branch 2 times, most recently from e79aeac to 0992376 Compare November 15, 2019 05:24
@robertf4 robertf4 force-pushed the add/single-product-backup-body-component branch from fe15730 to 277628c Compare November 15, 2019 05:28
@robertf4 robertf4 added [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. and removed [Status] Needs Author Reply We would need you to make some changes or provide some more details about your PR. Thank you! labels Nov 15, 2019
Copy link
Contributor

@delawski delawski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I left one minor comment in your code. Other than that the PR seems good to me so I'm approving.

Feel free to merge after addressing this one issue I commented on and after getting a thumbs-up from @jeherve.

@enejb enejb force-pushed the add/plan-price-display-component branch from 0992376 to ba2c6d1 Compare November 15, 2019 12:44
@robertf4 robertf4 force-pushed the add/plan-price-display-component branch from ba2c6d1 to 3674cbe Compare November 15, 2019 14:51
robertf4 and others added 6 commits November 15, 2019 08:54
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>
@robertf4 robertf4 force-pushed the add/single-product-backup-body-component branch from 4e109f7 to 285f8e0 Compare November 15, 2019 14:59
gravityrail and others added 2 commits November 15, 2019 17:03
* Allow TOS agreement before Jetpack is fully active so we track the connection flow

* Update packages/terms-of-service/src/class-terms-of-service.php

Co-Authored-By: Derek Smart <smart@automattic.com>

* Also update the action that is being called in jetpack

We renamed the action lets also rename it in Jetpack.

* [not verified] Revert "Also update the action that is being called in jetpack"

This reverts commit e9c0f21.


Co-authored-by: Brandon Kraft <public@brandonkraft.com>
Co-authored-by: Enej Bajgoric <enej.bajgoric@gmail.com>
This change allows the package to be used outside of Jetpack, relying on the methods provided by the Manager, plus using the WP_Error class instead of the Jetpack_Error wrapper.
@jeherve jeherve dismissed their stale review November 15, 2019 16:18

Comments addressed.

Copy link
Member

@jeherve jeherve left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me, barring @delawski's comment and my question about wording. Both can be addressed in a follow-up PR though. It should be good to merge.

_inc/client/plans/single-product-backup.jsx Show resolved Hide resolved
@jeherve jeherve added [Status] Ready to Merge Go ahead, you can push that green button! and removed [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. labels Nov 15, 2019
lezama and others added 4 commits November 15, 2019 16:04
* Adds spell checking and fixes files.

This implements a package to spellcheck files, and
does a pass to ensure everything is green.

* [not verified] Implements Travis check for spelling

Adds Yarn script and Travis CI for spell checking.

* [not verified] Fixes feedback.

Updates docs to prevent spelling exceptions for certain files.
@robertf4 robertf4 requested a review from a team as a code owner November 16, 2019 22:38
@gravityrail gravityrail merged commit d08f20a into add/plan-price-display-component Nov 16, 2019
@matticbot matticbot added [Status] Needs Changelog [Status] Needs Package Release This PR made changes to a package. Let's update that package now. and removed [Status] Ready to Merge Go ahead, you can push that green button! labels Nov 16, 2019
@jeherve jeherve removed [Status] Needs Changelog [Status] Needs Package Release This PR made changes to a package. Let's update that package now. labels Nov 18, 2019
@tyxla
Copy link
Member

tyxla commented Nov 18, 2019

I'm kind of concerned that we're merging these in branches different from master. What's the plan for moving them to master? I thought the plan was to have them based on master before merging.

@jeherve
Copy link
Member

jeherve commented Nov 18, 2019

It will be merged to master in #14056.

@tyxla
Copy link
Member

tyxla commented Nov 18, 2019

Awesome, thanks!

@kraftbj kraftbj deleted the add/single-product-backup-body-component branch January 4, 2021 22:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Admin Page React-powered dashboard under the Jetpack menu Plans Touches WP.com Files
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants