-
Notifications
You must be signed in to change notification settings - Fork 799
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
[WIP] Enable frontend preview for paid blocks requiring upgrade #18102
[WIP] Enable frontend preview for paid blocks requiring upgrade #18102
Conversation
Scheduled Jetpack release: January 12, 2021. Thank you for the great PR description! When this PR is ready for review, please apply the |
@@ -32,6 +31,7 @@ function register_block() { | |||
FEATURE_NAME, | |||
array( | |||
'render_callback' => __NAMESPACE__ . '\render_block', | |||
'enable_preview' => true, |
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 would love suggestions for a better name for this option.
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.
enable_frontend_preview
might be slightly more explanatory?
&& apply_filters( 'jetpack_block_editor_enable_upgrade_nudge', false ) | ||
/** This filter is documented in _inc/lib/admin-pages/class.jetpack-react-page.php */ | ||
&& apply_filters( 'jetpack_show_promotions', true ) | ||
&& ! is_feed() |
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.
The last two conditions here were part of the original check for rendering a frontend upgrade nudge. I am looking for more context to see if they are still needed, particularly the jetpack_show_promotions
filter.
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'm not seeing any usage of the jetpack_show_promotions
filter across the Jetpack or WordPress.com codebase. I suspect this is a global way to turn off all promotional notifications across Jetpack. It's possible that this is being used by hosts somewhere though, so best to leave it in.
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.
jetpack_block_editor_enable_upgrade_nudge
is the current way we enable the display of upgrade nudges on WordPress.com. Do we still need this with this work?
… jetpack/ prefix. This will let existing users transition without interruption.
…ia the child block functionality built into the Jetpack register block function.
…ted independently. This matches the ETK version of the block.
b7ae39a
to
838db4f
Compare
&& apply_filters( 'jetpack_block_editor_enable_upgrade_nudge', false ) | ||
/** This filter is documented in _inc/lib/admin-pages/class.jetpack-react-page.php */ | ||
&& apply_filters( 'jetpack_show_promotions', true ) | ||
&& ! is_feed() |
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'm not seeing any usage of the jetpack_show_promotions
filter across the Jetpack or WordPress.com codebase. I suspect this is a global way to turn off all promotional notifications across Jetpack. It's possible that this is being used by hosts somewhere though, so best to leave it in.
&& apply_filters( 'jetpack_block_editor_enable_upgrade_nudge', false ) | ||
/** This filter is documented in _inc/lib/admin-pages/class.jetpack-react-page.php */ | ||
&& apply_filters( 'jetpack_show_promotions', true ) | ||
&& ! is_feed() |
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.
jetpack_block_editor_enable_upgrade_nudge
is the current way we enable the display of upgrade nudges on WordPress.com. Do we still need this with this work?
// Stripe connection nudge. | ||
if ( ! membership_checks() && current_user_can_edit() ) { | ||
$stripe_nudge = render_stripe_nudge(); | ||
$stripe_nudge = render_stripe_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.
Missing ()
.
@@ -32,6 +31,7 @@ function register_block() { | |||
FEATURE_NAME, | |||
array( | |||
'render_callback' => __NAMESPACE__ . '\render_block', | |||
'enable_preview' => true, |
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.
enable_frontend_preview
might be slightly more explanatory?
This enables upgrade nudges in the editor, yes. It was part of the original conditional for displaying frontend upgrade nudges as well, so I included it in the first pass. I think you're right and it can be removed -- my thinking is: If upgrade nudges in the editor weren't enabled, the user wouldn't have been able to insert the block at all, so we should never end up in a scenario where we're rendering a paid block on the frontend of a free site that does not support nudges. |
Previously the Upgrade nudges were managed by the Premium Content blocks' render_callbacks. This will not work if the plan_check is enabled in the block at registration, because the render callback is wrapped in an availability check and won't be called. This commit abstracts the logic up a level. If the availability check fails, instead of returning immediately, we now check to see if upgrade nudges are enabled and the user is an admin. If so, we render an upgrade nudge in addition to the ouptut of the block's render_callback. The logic for rendering the upgrade nudges can also be removed from the Premium Content block now.
Adds an optional argument to `jetpack_register_block` to enable rendering a preview on the frontend for admins when upgrade nudges are enabled.
ce5ff8e
to
16b5b3c
Compare
Refactors the frontend upgrade nudge to a reusable SSR-rendable nudge that is used by the UpgradePlanBanner (the nudge used in the editor).
8c72c21
to
3ce3249
Compare
Closing in favor of #18239 |
Fixes 158-gh-Automattic/view-design and 157-gh-Automattic/view-design and 175-gh-Automattic/view-design.
Changes proposed in this Pull Request:
This PR re-enables frontend previews for paid blocks requiring an upgrade. If a block is unavailable due to not meeting plan requirements and the current user is an admin, the block will still be rendered on the frontend with an upgrade banner identical to the one that appears in the editor.
It also refactors the existing UpgradePlanBanner used in the editor to pull out as much common code as possible into a generic Nudge component that can be server-side-rendered.
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.
Follow testing instructions on #17907.
Note: the wp-env scripts will not sync changes to
class.jetpack-gutenberg.php
orclass-blocks.php
. These may need to be manually synced to their respective files in the sandbox.In order to test on free Jetpack sites, you will need to turn on the
jetpack_block_editor_enable_upgrade_nudge
filter locally.Proposed changelog entry for your changes: