-
Notifications
You must be signed in to change notification settings - Fork 802
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
Blocks: Show Blocks if they're missing a plan, add Upgrade Nudge #12823
Conversation
Caution: This PR has changes that must be merged to WordPress.com |
Thank you for the great PR description! When this PR is ready for review, please apply the Scheduled Jetpack release: August 6, 2019. |
ockham, Your synced wpcom patch D29733-code has been updated. |
2a42f87
to
c14d199
Compare
ockham, Your synced wpcom patch D29733-code has been updated. |
c14d199
to
c31a2ab
Compare
ockham, Your synced wpcom patch D29733-code has been updated. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good in general 👍
I haven't tested but left some initial feedback. I'm curious to hear your thoughts 🙂
<span className="upgrade-nudge__title"> | ||
{ sprintf( __( 'You need at least the following plan: %(plan)s', 'jetpack' ), { plan } ) } | ||
</span> | ||
<span className="upgrade-nudge__message">{ __( 'To gain access to this block.' ) }</span> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems like a single phrase that should be localized all together. Maybe the following?
To access this block, you need at least the following plan: %(plan)s
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
More copy discussion on this thread: paAmJe-ll-p2. Going with 9669ec3 for now.
import './style.scss'; | ||
|
||
export default ( { feature, plan } ) => ( | ||
<div className="upgrade-nudge"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cc @drw158 in case there's a common pattern that could eventually be extracted form this for wordpress/components
(card action? call to action? banner action?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just "Card" seems like a good primitive to have in the core?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably, yes.
ockham, Your synced wpcom patch D29733-code has been updated. |
1 similar comment
ockham, Your synced wpcom patch D29733-code has been updated. |
This way, the user will be able to insert the block, and will be prompted to upgrade to a plan that enables it.
ockham, Your synced wpcom patch D29733-code has been updated. |
ockham, Your synced wpcom patch D29733-code has been updated. |
ockham, Your synced wpcom patch D29733-code has been updated. |
|
||
import './style.scss'; | ||
|
||
const UpgradeNudge = ( { planName, planPathSlug, postId, postType } ) => ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see quite a long delay between "Upgrade" button pressed and the actual redirection. Is this something that we should resolve in this PR?
Also, this seems kinda expected, that upgrade flow will be loaded in the same tab, but in case of unsaved changes/updates - leaving the editor page is quite scary. Maybe we can trigger autosave or override default alert, to make sure folks are not confused?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree it's scary. We'll see how we can ease it, perhaps by saving a revision first and showing some core loading indicator.
Just saving edits made in already published posts would be problematic.
Material for another PR.
Not sure why, but I landed on the checkout page. with Premium plan added to my card. After checkout, I was redirected to the onboarding checklist, which I would not expect to see. We opening upgrade flow in the same editor's tab, so I would expect that flow will land me into my post editor. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Functionally tested, and it works kinda fine. I left a few comments regarding UX and differences with testing instructions, but I'm happy to follow-up in future PRs
Correct — this PR is a generic solution that can be extended for all premium blocks but implemented initially just for Simple payments.
Yep, as well as for VideoPress and WordAds but those will need little more work. See paAmJe-uG-p2 for context. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Didn't do another test after the last commit but reviewed code and all looks good. 👍
There will be follow-ups, this isn't meant to be perfect yet. :-)
That was changed last minute and was expected 👍
Heh, sorry, Calypso side is still pending merging. Automattic/wp-calypso#34440 :-) |
* Add initial changelog / testing list changes for 7.6 * Update stable tag to 7.5.3 * changelog: add #12957 * Changelog: add #12932 * Changelog: add #12867 * Changelog: add #12823 * changelog: add #12969 * changelog: add #13012 * changelog: add #12974 * Changelog: add #13059 * Changelog: add #13079 * Changelog: add #12924 * changelog: add #12954 * Changelog: add #12959 * Changelog: add #12977 * Changelog: add #12830 * Changelog: add #12926 * Changelog: add #12958 * Changelog: add #12999 * Changelog: add #13077 * Changelog: add #13083 * Changelog: add #13087 * Changelog: add #13110 * Changelog: add #13116 * Changelog: add #13117 * Changelog: add #12821 * Changelog: add #13120 * changelog: add #13139 * Changelog: add #13143 * Changelog: add #13147 * Testing list: add section about sync
* Add initial changelog / testing list changes for 7.6 * Update stable tag to 7.5.3 * changelog: add #12957 * Changelog: add #12932 * Changelog: add #12867 * Changelog: add #12823 * changelog: add #12969 * changelog: add #13012 * changelog: add #12974 * Changelog: add #13059 * Changelog: add #13079 * Changelog: add #12924 * changelog: add #12954 * Changelog: add #12959 * Changelog: add #12977 * Changelog: add #12830 * Changelog: add #12926 * Changelog: add #12958 * Changelog: add #12999 * Changelog: add #13077 * Changelog: add #13083 * Changelog: add #13087 * Changelog: add #13110 * Changelog: add #13116 * Changelog: add #13117 * Changelog: add #12821 * Changelog: add #13120 * changelog: add #13139 * Changelog: add #13143 * Changelog: add #13147 * Testing list: add section about sync
Picking up @scruffian's Automattic/wp-calypso#33723 and moving it into JP.
The rationale is that these blocks live in JP, so it's much easier to handle e.g. minimum plan information coming from the endpoint here. Furthermore, we might eventually want this upgrade nudge to also work in JP (where applicable!).
Changes proposed in this Pull Request:
Enable paid blocks on plans that don't include them for insertion into the editor, and add an upgrade nudge to them. Feature-flagged for now.
Is this a new feature or does it add/remove features to an existing part of Jetpack?
See paAmJe-il-p2
Screenshot
Styling pending some help from a designer 😬 :
Frontend (pre-existing, not touched by this PR, but will probably see some style and copy polish in a follow-up):
Testing instructions:
yarn build
), and start Docker (yarn docker:up
)docker/mu-plugins
, e.g. a newly created0-blocks-upgrade.php
(needs to start with<?php
):define ( 'JETPACK_SHOW_BLOCK_UPGRADE_NUDGE', true );
Once this PR has approval, I'll manually sync the newly created files over to the WP.com counterpart, D29733-code. Doing that now doesn't make sense since they'll be removed by Fusion each time this PR changes.
Proposed changelog entry for your changes:
Enable paid blocks on plans that don't include them for insertion into the editor, and add an upgrade nudge to them.
TODO
jetpack/modules/simple-payments/simple-payments.php
Lines 194 to 200 in ee0fcec
Use(not available inCard
component@wordpress/components
)Fix flow to return to editor after purchasesee below (under the 'Follow-up' heading)lib/plans/constants
since that file isn't available on WP.com -- only the contents of theextensions/
directory are (on the client side, anyway). However, we have access to the REST API, so maybe we can fetch information from there./plans
endpoint which should provide the relevant information, so I'm thinking of using@wordpress/data
andapiFetch
to obtain the relevant strings (queryproduct_slug
orpath_slug
field, getproduct_name_short
field).Don't forget that this is WP.com first -- we'll probably even add a check to hide it from JP for now. This is mostly relevant for the upgrade flow.
Follow-up
checkout-thank-you/index.jsx
is a likely candidate for those changes, see e.g.redirectIfThemePurchased
. We'll probably have to pass a query arg or cookie./cc @simison @sirreal