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 Jetpack Backup Daily and Real-time to My Plan page #13756

Merged
merged 11 commits into from
Oct 18, 2019
Merged

Add Jetpack Backup Daily and Real-time to My Plan page #13756

merged 11 commits into from
Oct 18, 2019

Conversation

robertf4
Copy link
Contributor

@robertf4 robertf4 commented Oct 14, 2019 •
edited
Loading

Changes proposed in this Pull Request:

This adds support for the new Jetpack Backup products to the My Plan page.

Changes:

  1. Update header title for Jetpack Backup plans.
  2. Update Backup card to correctly display Jetpack Backup plans.

Screenshots will be added once I have the final SVG.

Is this a new feature or does it add/remove features to an existing part of Jetpack?

This updates the My Plan page to support the new Jetpack Backup product.

See the P2 here for the design of this PR: p1HpG7-7M8-p2
See the P2 here for the overall MT: p1HpG7-7ET-p2

Testing instructions:

The only way to test these products is to fake the data in Redux until the product is available to purchase in the wpcom store (see for status of that dev see p1HpG7-7Dj-p2).

To fake the data use the Dev Tools to choose the "Jetpack Backup Daily" or "Jetpack Backup Real-time" plans.

To test:

  1. View the My Plan page for a free plan and some current Jetpack paid plans. Test the upgrade CTA in the header and check that no cards are missing.
  2. View the My Plan page for each of the faked Jetpack Backup plans listed above. Test the upgrade CTA in the header, check that the Backup card is displayed and that no cards are missing, and check that the Backup card link works.

Proposed changelog entry for your changes:

  • Updated My Plan page to support Jetpack Backup Daily and Jetpack Backup Real-time

@robertf4 robertf4 requested a review from a team October 14, 2019 21:54
@jetpackbot
Copy link

jetpackbot commented Oct 14, 2019 •
edited
Loading

Thank you for the great PR description!

When this PR is ready for review, please apply the [Status] Needs Review label. If you are an a11n, please have someone from your team review the code if possible. The Jetpack team will also review this PR and merge it to be included in the next Jetpack release.

Scheduled Jetpack release: November 5, 2019.
Scheduled code freeze: October 29, 2019

Generated by 🚫 dangerJS against f4afb69

@jeherve jeherve added Admin Page React-powered dashboard under the Jetpack menu Plans labels Oct 15, 2019
@robertf4 robertf4 force-pushed the update/add-single-purpose-jetpack-backup-product-to-my-plan branch from 28f8abf to fc12c77 Compare October 15, 2019 19:24
@robertf4
Copy link
Contributor Author

@jeherve do you know how to mark a codeclimate issue as won't fix? It's marking my use of PlanHeaderCard as excessive duplication when it isn't as I am simply using the component.

@jeherve
Copy link
Member

jeherve commented Oct 16, 2019

do you know how to mark a codeclimate issue as won't fix?

It's okay to ignore it. It won't block the merge. We're still in the process of fine-tuning those CodeClimate rules so it's not always super useful. See #13468 for more information.

It's marking my use of PlanHeaderCard as excessive duplication when it isn't as I am simply using the component.

That's probably because very few things are different between the is-daily-backup-plan case and the is-realtime-backup-plan one.

@robertf4 robertf4 added this to the 7.9 milestone Oct 16, 2019
@robertf4 robertf4 added [Status] Ready to Merge Go ahead, you can push that green button! [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. and removed [Status] In Progress [Status] Ready to Merge Go ahead, you can push that green button! labels Oct 16, 2019
@jeherve jeherve added the [Status] Needs Design Review Design has been added. Needs a review! label Oct 17, 2019
@jeherve
Copy link
Member

jeherve commented Oct 17, 2019

I added the new plans to the Dev tools in 1c10645 to make this a bit easier to test.

You should now be able to switch from one plan to the next like so:
image

jeherve
jeherve requested changes Oct 17, 2019
View reviewed changes
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 works well for me. I only have a few remarks / questions:

The new icon does not seem to be aligned with the other one next to it:

image

Test the upgrade CTA in the header

Right now this seems to link to the generic plans page. Will that change later?

_inc/client/my-plan/my-plan-header/index.js Show resolved Hide resolved
@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 fellow Jetpack developers. Label will be renamed soon. labels Oct 17, 2019
@robertf4
Copy link
Contributor Author

robertf4 commented Oct 17, 2019 •
edited
Loading

I added the new plans to the Dev tools in 1c10645 to make this a bit easier to test.

Awesome, I didn't know that existed. Thanks!

The new icon does not seem to be aligned with the other one next to it

I think this may be because this icon is from a new set that has a new height, or perhaps I exported it incorrectly. I have a comment asking for clarification at p1HpG7-7M8#comment-34870 and if I get an updated icon I will do another PR to update it. I'll follow up on that today with a ping once Chris is online.

This is now fixed, see Chris's comment below.

Right now this seems to link to the generic plans page. Will that change later?

That is the plan, see p1HpG7-7M8#comment-34877 for the discussion on that. So testing the CTA here really just means making sure the source/target in the URL are unique, as if they are unique we can easily update the redirect at any point. I'll update the testing instructions to better reflect this.

@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 Oct 17, 2019
@crunnells
Copy link
Contributor

crunnells commented Oct 17, 2019 •
edited
Loading

The new icon does not seem to be aligned with the other one next to it:

Fixed the icon size in a43a66f

jeherve
jeherve approved these changes Oct 18, 2019
View reviewed changes
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 and tests well to me. It should be good to merge!

@jeherve jeherve added [Status] Ready to Merge Go ahead, you can push that green button! and removed [Status] Needs Design Review Design has been added. Needs a review! [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. labels Oct 18, 2019
@robertf4 robertf4 merged commit 48f50d5 into master Oct 18, 2019
@robertf4 robertf4 deleted the update/add-single-purpose-jetpack-backup-product-to-my-plan branch October 18, 2019 13:16
@matticbot matticbot added [Status] Needs Changelog and removed [Status] Ready to Merge Go ahead, you can push that green button! labels Oct 18, 2019
@jeherve jeherve added [Status] Needs Testing We need to add this change to the testing call for this month's release [Status] Has Changelog and removed [Status] Needs Changelog [Status] Needs Testing We need to add this change to the testing call for this month's release labels Oct 18, 2019
@jeherve jeherve mentioned this pull request Oct 23, 2019
Closed
jeherve added a commit that referenced this pull request Oct 23, 2019
@jeherve
Changelog: add #13756
43b27f0
jeherve added a commit that referenced this pull request Oct 29, 2019
@jeherve
7.9: Changelog (#13608)
6cd1621 
* 7.9: Changelog

* Update version number

* Update stable tag and tested up to

* Changelog: add #13530

* changelog: add #13578

* Changelog: add #13598

* Changelog: add entry for numerous block preview changes

* Changelog: add #13599

* changelog: add #13541

* Changelog: add #13542

* Changelog: add #13331

* Changelog: add #13558

* Changelog: add #13409

* Changelog: add #13582

* Changelog: add #13600

* Changelog: add #13601

* Changelog: add #13595

* Changelog: add #12695

* Changelog: add #13009

* Changelog: add #13649

* Changelog: add #13450

* Changelog: add #13507

* Changelog: add #13658

* Changelog: add #13687

* changelog: add #13683

* Changelog: add #9323

* Changelog: add #13681

* Fix typos in readme

* Add link to WordPress Beta Tester plugin

* Changelog: add #13630

* Changelog: add #13695

* Changelog: add #13659

* Changelog: add #13716

* Changelog: add #13664

* Changelog: add #13682

* Changelog: add #13362

* Changelog: add #13563

* Add testing list for #13563

* Changelog: add #13735

* Changelog: add #13752

* Changelog: add #13624

* Changelog: add #13756

* Changelog: add #13745

* Changelog: add #13728

* Changelog: add #13779

* Changelog: add #13699

* Changelog: add #13804

* Changelog: add #13761

* Changelog: add #13637

* Changelog: add #13517

* Changelog: add #13521

* Changelog: add #13729

* Testing list: add testing instructions for #13729

* Changelog: add sync changes

* Changelog: add #13807

* Changelog: add #13654

* Changelog: add #13795

* Changelog: add #13801

* Changelog: add #13818

* Changelog: add #13725

* Changelog: add #13831

* Changelog: add #13516

* Testing list: add Twenty Twenty instructions

* Changelog: add #13799

* Changelog: add #13805

* Changelog: add #13688

* Changelog: add #13830
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jeherve jeherve jeherve approved these changes

Assignees

@robertf4 robertf4

Labels
Admin Page React-powered dashboard under the Jetpack menu [Feature] Backup & Scan Plans
Projects
None yet
Milestone
7.9
Development

Successfully merging this pull request may close these issues.
5 participants
@robertf4 @jetpackbot @jeherve @crunnells @matticbot