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

Plans: Add Single Product Jetpack Backup #14018

Merged

robertf4 merged 28 commits into master from add/single-product-jetpack-backup-to-plans-page

Nov 18, 2019

Contributor

robertf4 commented Nov 12, 2019

Changes proposed in this Pull Request:

This PR adds a card to the Plans page to sell Jetpack Backup.

Current development:

Figma design:

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:

Go to the Plans page on a Jetpack site with a free plan.
Verify that the Jetpack Backup card shows.
Verify that you can toggle between realtime and daily.
Verify that the upgrade button works.

Proposed changelog entry for your changes:

Added upgrade path for Jetpack Backup to Plans page.

robertf4 added [Status] In Progress Admin Page Plans labels

robertf4 requested a review from a team as a code owner

November 12, 2019 04:24

robertf4 changed the title ~~Add/single product jetpack backup to plans page~~ Plans: Add Single Product Jetpack Backup

jetpackbot commented Nov 12, 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 6715027

robertf4 changed the base branch from master to add/products-data-component

November 12, 2019 04:35

robertf4 added [Status] Needs Review and removed [Status] In Progress labels

Contributor Author

robertf4 commented Nov 12, 2019

Note to reviewers: differences between this PR and #13766:

I integrated the actual server data
Everything should be I18N'd now

robertf4 mentioned this pull request

Add Jetpack Backup Daily and Real-time to Plans page #13766

Closed

tyxla requested changes

View reviewed changes

Member

tyxla left a comment

Hey @robertf4, nice work! This appears to work well in my testing.

I really appreciate all the great progress here, but I still think this PR could be split to some more PRs - especially all the presentational components that this PR is introducing.

I've also left some suggestions, questions and feedback, let me know what you think.

_inc/client/plans/index.jsx Show resolved Hide resolved

_inc/client/plans/single-product-backup.jsx Outdated

    
              	);

              }

              export function PlanPriceDisplay( props ) {

Member

tyxla Nov 12, 2019

Can we implement all of those components in their own PRs? I'm concerned that when they're in those big PRs they can be overlooked. I'm happy to help with reviewing all those PRs and whetever else you might need.

Contributor Author

robertf4 Nov 14, 2019

I put each component in its own PR except for SingleProductBackupHeader, which I left here since it is pretty small.

#14025
#14026
#14027

_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 Outdated

    
              					onChange={ onChange }

              				/>

              			</div>

              			<div className="plan-radio-button__plan-name">{ planName }</div>

Member

tyxla Nov 12, 2019

None of these are currently usable enough - they should all be labels, so when I click on them, we select the corresponding radio buttons.

_inc/client/plans/single-product-backup.jsx Outdated

    
              			<div className="plan-radio-button__plan-name">{ planName }</div>

              			<div className="plan-radio-button__plan-price">

              				{ planPrice &&

              					__( '%(currencySymbol)s%(planPrice)s /year', {

Member

tyxla Nov 12, 2019

Some of those components aren't prepared for monthly support. Others seem to be prepared for that. I find this pretty confusing - are we planning to have monthly product support? If not, should we omit any monthly mention in this PR? If yes, should we implement it?

Contributor Author

robertf4 Nov 13, 2019

The only time monthly is currently used is for calculating the larger price as monthly * 12. It is necessary to display the larger price correctly, but I am not using monthly for anything else.

_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

jeherve added [Status] Needs Author Reply and removed [Status] Needs Review labels

jeherve assigned robertf4

jeherve added this to the 8.0 milestone

jeherve added the [Status] Needs Design Review label

keoshi reviewed

View reviewed changes

Contributor

keoshi left a comment •

edited

Overall this looks good to me. I have a few notes though:

Different typefaces and composition between adjacent elements

Top headings are using system fonts, the bottom is using Roboto.

Not enough room for elements to breathe

In particular, there's no room between the backups box and the monthly/yearly toggle.

Monthly/Yearly toggle doesn't affect Jetpack Backup pricing

Different currency displayed in product/plans

Different price formatting: `/year` vs `per year`

Page looks too pyramid-like, with no balance at the top

Plans are not displayed on mobile but single products are, making the headline a bit confusing

robertf4 force-pushed the add/products-data-component branch from ea23c3f to 5319df0 Compare

November 14, 2019 05:12

robertf4 force-pushed the add/single-product-jetpack-backup-to-plans-page branch from 906af0f to b3543e1 Compare

November 14, 2019 05:23

robertf4 mentioned this pull request

Add PlanRadioButton Component #14025

Merged

robertf4 force-pushed the add/single-product-jetpack-backup-to-plans-page branch from b3543e1 to 1d13019 Compare

November 14, 2019 05:41

robertf4 changed the base branch from add/products-data-component to add/plan-radio-button

November 14, 2019 05:41

robertf4 mentioned this pull request

Add PlanPriceDisplay Component #14026

Closed

robertf4 force-pushed the add/single-product-jetpack-backup-to-plans-page branch from 1d13019 to 09c8d67 Compare

November 14, 2019 05:58

robertf4 changed the base branch from add/plan-radio-button to add/products-data-component

November 14, 2019 05:58

robertf4 changed the base branch from add/products-data-component to add/plan-price-display-component

November 14, 2019 06:00

robertf4 mentioned this pull request

Add SingleProductBackupBody Component #14027

Merged

robertf4 force-pushed the add/single-product-jetpack-backup-to-plans-page branch from 09c8d67 to 01cc0ab Compare

November 14, 2019 06:19

robertf4 changed the base branch from add/plan-price-display-component to add/single-product-backup-body-component

November 14, 2019 06:19

robertf4 and others added 16 commits

November 18, 2019 10:33


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

b1bee3a

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


          Remove duplicate definition of getProducts

658f8ed


          Added yearly single product backup upgrade prompt to the Plans page i…

9a7b015

…n wp-admin


          Change 'Single Products' to 'Solutions'

b8008c8


          Switched to quotes

331e817


          Factor out calculation of full yearly prices and redo I18N of that

9ce1820


          Make more robust against products not existing

fedfcf6


          Convert SingleProductBackupBody to class component

cff63b6


          Rename plans-price__span to plans-price__price-range

4e6f9f0


          Change slashed to discounted

7efb572


          Destructure props instead of declaring a props parameter

3362ba9


          Changed upgrade links to use getUpgradeUrl

493d09f


          Flatten CSS class names

b015600


          Rename single-product-backup__accented-card-container to plans-sectio…

9e919b6

…n__single-product


          Add single-product-backup__header-title CSS class

a2d4b36


          Fix merge issues

robertf4 force-pushed the add/single-product-jetpack-backup-to-plans-page branch from 195f8df to 6715027 Compare

November 18, 2019 16:48

Contributor Author

robertf4 commented Nov 18, 2019

@jeherve @tyxla this is now rebased against master.

jeherve added [Status] Needs Review and removed [Status] Needs Author Reply labels

jeherve approved these changes

View reviewed changes

Member

jeherve left a comment

This looks good to me. Currency management may need to be revisited as discussed in #14060, but that can be done in a future PR 👍

Feel free to merge that one!

jeherve added [Status] Ready to Merge and removed [Status] Needs Review labels

robertf4 dismissed tyxla’s stale review

November 18, 2019 18:26

Further issues to be addressed in followup PRs

robertf4 merged commit 4f591d0 into master

robertf4 deleted the add/single-product-jetpack-backup-to-plans-page branch

November 18, 2019 18:27

matticbot added [Status] Needs Changelog and removed [Status] Ready to Merge labels

jeherve removed [Status] Needs Changelog [Status] Needs Design Review labels

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment