Subscription Site: Allow to add Subscribe block at the end of each post#35458
Conversation
|
Are you an Automattician? Please test your changes on all WordPress.com environments to help mitigate accidental explosions.
Interested in more tips and information?
|
|
Thank you for your PR! When contributing to Jetpack, we have a few suggestions that can help us test and review your patch:
This comment will be updated as you work on your PR and make changes. If you think that some of those checks are not needed for your PR, please explain why you think so. Thanks for cooperation 🤖 The e2e test report can be found here. Please note that it can take a few minutes after the e2e tests checks are complete for the report to be available. Once your PR is ready for review, check one last time that all required checks appearing at the bottom of this PR are passing or skipped. Jetpack plugin: The Jetpack plugin has different release cadences depending on the platform:
If you have any questions about the release process, please ask in the #jetpack-releases channel on Slack. |
| .wp-block-jetpack-subscriptions__subscribe_post_end { | ||
| margin: 2em auto !important; | ||
| gap: 0 !important; | ||
|
|
||
| & > p { | ||
| margin-bottom: 0.8em !important; | ||
| } | ||
| } |
There was a problem hiding this comment.
Those custom styles are used only for classic themes.
There was a problem hiding this comment.
Might be worth a comment to clarify it's for classic themes and in what scenario. Hard to know otherwise.
There was a problem hiding this comment.
Good idea, added the comment.
| disableInSiteConnectionMode | ||
| module={ subscriptions } | ||
| > | ||
| <p>Enable automatic insertion of the Subscribe block into the theme</p> |
There was a problem hiding this comment.
Decided to not wrap it with __ to not bother translators since it still may change.
| 'jetpack_subscriptions_subscribe_post_end_enabled', | ||
| ] ) } | ||
| onChange={ handleSubscribePostEndToggleChange } | ||
| label="End of each post" |
There was a problem hiding this comment.
No __ as well for the same reason as above.
| 'attrs' => $attrs, | ||
| 'innerBlocks' => array( $hooked_block ), | ||
| 'innerContent' => array( | ||
| '<div class="wp-block-group"> |
There was a problem hiding this comment.
If we still need the group block for layout purposes, let's use the full block markup version:
<!-- wp:group {"layout":{"type":"constrained"}} -->
<div class="wp-block-group">
</div>
<!-- /wp:group -->There was a problem hiding this comment.
Oh I see now, it comes from the above array with core/group etc. Do we then still need the div or doesn't that get generated on its own?
There was a problem hiding this comment.
It looks like it's redundant but it seems it's needed. It's not getting generated automatically, the same for:
<!-- wp:paragraph {"style":{"typography":{"fontStyle":"normal","fontWeight":"300"}},"className":"has-text-align-center"} -->
<p class="has-text-align-center" style="font-style:normal;font-weight:300">
If you look at our themes, for example: https://github.com/Automattic/themes/blob/trunk/lettre/templates/index.html you'll find similar things.
There was a problem hiding this comment.
The solution is taken from @ockham like button 😄
https://github.com/ockham/like-button/blob/2520519633b27d2edaf0f7a58f4161ebc17f36c6/like-button.php#L45-L55
simison
left a comment
There was a problem hiding this comment.
Let's get this in 👍
I didn't test with latest WP trunk, and instead only had latest trunk of Gutenberg plugin. Hence the wrapper blocks didn't work for me, and the result wasn't good looking.
We could add Gutenberg & WP core version checks to ensure the feature works well? Jetpack itself supports two previous WP versions.
Feature flag worked well for toggle and the hook.
Another thing that might require likes/sharing to be blocks is that would make sense for subscribe block be above those buttons:
| <p class="has-text-align-center" style="font-style:normal;font-weight:300"> | ||
| <em>Aliquam a ullamcorper lorem<br>Integer at tempus nibh</em> | ||
| </p> | ||
| <!-- /wp:paragraph -->', |
There was a problem hiding this comment.
I thought the Subscribe block would come here but is that also getting automatically added by the hook itself?
There was a problem hiding this comment.
It's passd by innerBlocks:
'innerBlocks' => array( $hooked_block ),
'innerContent' => array(
'<div class="wp-block-group">
<!-- wp:paragraph {"style":{"typography":{"fontStyle":"normal","fontWeight":"300"}},"className":"has-text-align-center"} -->
<p class="has-text-align-center" style="font-style:normal;font-weight:300">
<em>Aliquam a ullamcorper lorem<br>Integer at tempus nibh</em>
</p>
<!-- /wp:paragraph -->',
null,
'</div>',and it's automatically inserted into the innerContent.
|
An example for version check in https://github.com/Automattic/jetpack/pull/35905/files#diff-afc4158db428ae314674d2fab477ce8ba67bfae66be4987e32e026ebf58952ccR40 |
| * @return bool | ||
| */ | ||
| protected function is_subscription_site_feature_enabled() { | ||
| return (bool) apply_filters( 'jetpack_subscription_site_enabled', false ); |
There was a problem hiding this comment.
You'll want to add a docblock for this filter, so the codex parser can pick it up.
There was a problem hiding this comment.
Not sure why we need it but added. This is a feature flag and we don't want to expose it.
There was a problem hiding this comment.
@jeherve fine to keep the filter internal? We wouldn't want to use filter deprecation when removing it.
There was a problem hiding this comment.
I suppose that would be okay. I guess I'm just a bit weary of filters that are temporary but that end up staying for some time. :)
Let's maybe add a short comment saying that the filter is temporary?
| add_filter( | ||
| 'hooked_block_types', | ||
| function ( $hooked_blocks, $relative_position, $anchor_block ) { | ||
| if ( $anchor_block === 'core/post-content' && $relative_position === 'after' ) { | ||
| $hooked_blocks[] = 'jetpack/subscriptions'; | ||
| } | ||
|
|
||
| return $hooked_blocks; | ||
| }, | ||
| 10, | ||
| 3 | ||
| ); | ||
|
|
||
| add_filter( |
There was a problem hiding this comment.
Since those are brand new hooks that have changed a bit since WordPress 6.4 came out, I personally opted to gate the auto-addition to WP 6.5+, see #35905
There was a problem hiding this comment.
Good idea, added, thanks!
Related to peKye1-EA-p2
Proposed changes:
It:
The feature is behind the feature flag and if you want to enable it you need to go into your sandbox mu-plugins, and add the following line to your
0-sandbox.phpfile:Other information:
Jetpack product discussion
Does this pull request change what data or activity we track or use?
Testing instructions:
Prerequisites
You need to have Jetpack Newsletter module enabled.
Feature flag disabled
Feature flag enabled