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.

Sign up for GitHub

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

Jump to bottom

Add PlanPriceDisplay Component #14026

Closed
wants to merge 18 commits into from
Closed

Add PlanPriceDisplay Component #14026

wants to merge 18 commits into from

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:

This cannot be tested on its own here. Use #14018 to test.

Proposed changelog entry for your changes:

  • No changelog needed

@robertf4 robertf4 requested a review from a team as a code owner November 14, 2019 05:53
@robertf4 robertf4 self-assigned this Nov 14, 2019
@robertf4 robertf4 changed the base branch from add/products-data-component to add/plan-radio-button November 14, 2019 06:00
@robertf4 robertf4 changed the title Add/plan price display component Add PlanPriceDisplay Component Nov 14, 2019
@robertf4 robertf4 added [Status] Needs Review To request a review from Crew. Label will be renamed soon. Admin Page React-powered dashboard under the Jetpack menu Plans and removed [Status] In Progress labels Nov 14, 2019
@robertf4 robertf4 mentioned this pull request Nov 14, 2019
Merged
delawski
delawski suggested changes Nov 14, 2019
View reviewed changes
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 Thanks for your work on this. I left some comments in the code.

I know you've got a lot on your plate right now, so if you need any support in addressing the feedback, feel free to reach out to me.

_inc/client/plans/single-product-backup.jsx 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 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 Crew. Label will be renamed soon. labels Nov 14, 2019
@jeherve jeherve added this to the 8.0 milestone Nov 14, 2019
jeherve
jeherve previously requested changes Nov 14, 2019
View reviewed changes
_inc/client/plans/single-product-backup.jsx Outdated Show resolved Hide resolved
@robertf4 robertf4 force-pushed the add/plan-price-display-component branch 3 times, most recently from e79aeac to 0992376 Compare November 15, 2019 05:24
@robertf4 robertf4 added [Status] Needs Review To request a review from Crew. 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
delawski
delawski previously approved these changes Nov 15, 2019
View reviewed changes
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.

Thank you for updating the code.

I'm approving it as is, also keeping in mind that we'll still have to refactor the price blocks in one of the next PRs.

@enejb enejb force-pushed the add/plan-price-display-component branch from 0992376 to ba2c6d1 Compare November 15, 2019 12:44
@robertf4 robertf4 requested a review from a team as a code owner November 15, 2019 12:44
@enejb enejb changed the base branch from add/plan-radio-button to master 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
@jetpackbot
Copy link

jetpackbot commented Nov 15, 2019 •
edited

Warnings
⚠️

pre-commit hook was skipped for one or more commits

This is an automated check which relies on PULL_REQUEST_TEMPLATE. We encourage you to follow that template as it helps Jetpack maintainers do their job. If you think 'Testing instructions' or 'Proposed changelog entry' are not needed for your PR - please explain why you think so. Thanks for cooperation 🤖

Generated by 🚫 dangerJS against f126b14

@jeherve jeherve 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 Crew. Label will be renamed soon. labels Nov 15, 2019
@jeherve
Copy link
Member

jeherve commented Nov 15, 2019

pre-commit hook was skipped for one or more commits

Do you think you could run yarn build in the environment where you commit, to avoid these errors and benefit from the things our pre-commit hook brings us, such as linting and prettyfying on commit?

@robertf4
Copy link
Contributor Author

Do you think you could run yarn build in the environment where you commit, to avoid these errors and benefit from the things our pre-commit hook brings us, such as linting and prettyfying on commit?

I do this. For this case, because I split out each component into its own PR, the components were unused which gives an error that prevented me from committing. I only committed when this was the only error I got.

@robertf4 robertf4 added [Status] Needs Review To request a review from Crew. 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
@robertf4 robertf4 mentioned this pull request Nov 15, 2019
Closed
gravityrail
gravityrail previously approved these changes Nov 15, 2019
View reviewed changes
Copy link
Contributor

@gravityrail gravityrail left a comment

Choose a reason for hiding this comment

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

This is good to go IMO. A few follow-up PRs but nothing that should block this.

@robertf4 @gravityrail
Add SingleProductBackupBody Component (#14027)
d08f20a 
* Add products endpoint

* [not verified] Add error code

* Update _inc/lib/class.core-rest-api-endpoints.php

Co-Authored-By: Marin Atanasov <8436925+tyxla@users.noreply.github.com>

* Update _inc/lib/class.core-rest-api-endpoints.php

Co-Authored-By: Marin Atanasov <8436925+tyxla@users.noreply.github.com>

* Add QueryProducts data component and corresponding Redux and rest API calls

* Update _inc/client/state/site/reducer.js

Co-Authored-By: Marin Atanasov <8436925+tyxla@users.noreply.github.com>

* [not verified] Add SingleProductBackupBody component

* [not verified] Rename plans-section__body to single_product_backup__body

* [not verified] Give class name to h4 tag

* [not verified] Remove single_product_backup__body

* [not verified] Check for upgradeLinks and upgradeTitles before using them

* [not verified] Switch function style

* [not verified] Add key to PlanRadioButton

* Remove duplicate definition of getProducts

* Allow TOS agreement before Jetpack is fully active so we track… (#14041)

* 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>

* Removed Jetpack references in the IXR client. (#14046)

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.

* Full Sync: Don't allow more than one request to enqueue (#14039)

* Spell checking CI integration (#13992)

* 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.

* Export SingleProductBackupBody class

* [not verified] No need to export single product backup body
@matticbot
Copy link
Contributor

Caution: This PR has changes that must be merged to WordPress.com
Hello robertf4! These changes need to be synced to WordPress.com - If you 're an a11n, please commandeer, review, and approve D35575-code before merging this PR. Thank you!

@gravityrail gravityrail dismissed jeherve’s stale review November 16, 2019 23:11

String now translated

gravityrail
gravityrail previously approved these changes Nov 16, 2019
View reviewed changes
Copy link
Contributor

@gravityrail gravityrail left a comment

Choose a reason for hiding this comment

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

Good to go

@jeherve
Copy link
Member

jeherve commented Nov 18, 2019

We can probably close this PR in favor of #14056.

@jeherve jeherve closed this Nov 18, 2019
@jeherve jeherve deleted the add/plan-price-display-component branch November 27, 2019 16:15
@kraftbj kraftbj removed the [Status] Needs Review To request a review from Crew. Label will be renamed soon. label Sep 29, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gravityrail gravityrail gravityrail left review comments

@jeherve jeherve Awaiting requested review from jeherve

@delawski delawski Awaiting requested review from delawski

Assignees

@robertf4 robertf4

Labels
Admin Page React-powered dashboard under the Jetpack menu Plans
Projects
None yet
Milestone
8.0
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@robertf4 @jetpackbot @jeherve @matticbot @gravityrail @delawski @kraftbj