-
Notifications
You must be signed in to change notification settings - Fork 793
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
Update/enable frontend upgrade nudges #18239
Conversation
Scheduled Jetpack release: February 2, 2021. Thank you for the great PR description! When this PR is ready for review, please apply the |
bc73df1
to
4eab5db
Compare
4366d18
to
5387648
Compare
6332b89
to
3039f1e
Compare
Testing Instructions for Atomic (so Stripe connecting works):
|
I've gone through a simple site, atomic site, free and paid. I've checked all existing paid blocks. Nice work! This all worked very well except for one thing -- the Payments block seems to not display the notice on the front end. It also shows up in an incognito window, but that's also the case on master. |
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, the only thing I see is to clean up the missing period marks at the end of comments that the linter is complaining about.
@apeatling Thanks for pointing this out. Do you mean the Recurring Payments block? That block is unfortunately different because it does not use the I've crossed that block out in the list of paid blocks to test. Do you think it's worth a follow-up to add this to Payments for consistency? This is yet another annoying sticking point caused by that block's custom registration. |
@stacimc Can you make an issue as a follow up? No need to work on it for this PR. |
3039f1e
to
13ab974
Compare
Do you think we could merge #17907 first, and then focus on this one? I think it would help, as the original Premium content block is getting a bit unwieldy. |
863d9ed
to
c66d30a
Compare
Caution: This PR has changes that must be merged to WordPress.com |
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. I pushed a few changes, tested, this should be good to merge. I spotted some minor issues, but nothing blocking.
On WordPress.com Simple
I tested with D55853-code, in Chrome.
- On Free sites, I think we could do with some extra padding in the upgrade nudge for the Payments button inside the Premium Content block:
- Once I've paid for a plan and connected to Stripe, I did not see the Payment button on the frontend, only in the editor. The problem disappeared when I created a Payment button outside of the Premium Content block and assigned a plan to the button. I suppose that's because the subscription plan was saved.
- The same thing happened in Atomic.
On Atomic
I didn't spot any issues.
On Jetpack
I can't see the block when only using the production bundle. 👍
projects/plugins/jetpack/extensions/shared/components/upgrade-nudge/style.scss
Show resolved
Hide resolved
Going to merge this now. I'll let you handle the WordPress.com counterpart with the other related diffs later :) Do you want me to create issues for the comments I left above? |
@jeherve If you don't mind that would be great; I'm not positive I understand exactly how to repro. Thank you! |
r220427-wpcom |
Fixes 158-gh-Automattic/view-design and 157-gh-Automattic/view-design and 175-gh-Automattic/view-design.
Changes proposed in this Pull Request:
Upgrade nudge
Stripe nudge
On Atomic sites, the Stripe nudge redirects the user back to the post in the editor in order to edit the block/connect Stripe, rather than connecting directly to Stripe checkout
Jetpack product discussion
Internal reference: pcjTuq-8U-p2
Does this pull request change what data or activity we track or use?
Testing instructions:
This requires re-testing across all the scenarios: wpcom and Jetpack, with and without the required plan, and with and without Stripe connected.
Test setup notes
class.jetpack-gutenberg.php
,components.php
, orclass-blocks.php
. These may need to be manually synced to their respective files in the sandbox.npm run build
from within the Jetpack dir in order to build the SSR templates.Test scenarios
Jetpack
Wpcom
Atomic (need to use business site so Stripe access works)
Other Paid Blocks (Wpcom)
(Pay with Paypal, Donations,
Payments,Video, WhatsApp, Open Table, Calendly)Proposed changelog entry for your changes: