-
Notifications
You must be signed in to change notification settings - Fork 801
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
Subscriptions: Add Welcome Overlay behind the feature flag #37372
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. |
.../plugins/jetpack/modules/subscriptions/subscribe-overlay/class-jetpack-subscribe-overlay.php
Outdated
Show resolved
Hide resolved
.../plugins/jetpack/modules/subscriptions/subscribe-overlay/class-jetpack-subscribe-overlay.php
Outdated
Show resolved
Hide resolved
.../plugins/jetpack/modules/subscriptions/subscribe-overlay/class-jetpack-subscribe-overlay.php
Outdated
Show resolved
Hide resolved
projects/plugins/jetpack/modules/subscriptions/subscribe-overlay/subscribe-overlay.css
Outdated
Show resolved
Hide resolved
projects/plugins/jetpack/modules/subscriptions/subscribe-overlay/subscribe-overlay.css
Show resolved
Hide resolved
.../plugins/jetpack/modules/subscriptions/subscribe-overlay/class-jetpack-subscribe-overlay.php
Show resolved
Hide resolved
public function enqueue_assets() { | ||
if ( $this->should_user_see_overlay() ) { | ||
wp_enqueue_style( 'subscribe-overlay-css', plugins_url( 'subscribe-overlay.css', __FILE__ ), array(), JETPACK__VERSION ); | ||
wp_enqueue_script( 'subscribe-overlay-js', plugins_url( 'subscribe-overlay.js', __FILE__ ), array( 'wp-dom-ready' ), JETPACK__VERSION, 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.
Wondering if we should add the subscriptions module to webpack, build the frontend JS as well and include the JS from jetpack/projects/plugins/jetpack/_inc/build
?
Would help with minification and also allow importing from @wordpress/dom-ready
directly, and build pipeline "externalizes" WP scripts instead of including them in bundles.
No strong opinion and current way is fine too.
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.
Seems like an idea for a separate PR.
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.
Yep sounds good 👍
.../plugins/jetpack/modules/subscriptions/subscribe-overlay/class-jetpack-subscribe-overlay.php
Outdated
Show resolved
Hide resolved
.../plugins/jetpack/modules/subscriptions/subscribe-overlay/class-jetpack-subscribe-overlay.php
Outdated
Show resolved
Hide resolved
$template->status = 'publish'; | ||
$template->has_theme_file = false; | ||
$template->is_custom = true; | ||
$template->description = __( 'An overlay that shows up when someone visits your site', 'jetpack' ); |
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.
Is the copy consistent with the toggle setting label from designs?
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.
Good question. The toggle label is:
Which IMO is missing the verb, like "Enable" or sth. Now it's a bit mysterious to me, @crisbusquets WDYT?
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.
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.
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 description needs .
at the end.
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.
Added, the subscribe modal and modal for comments don't have the dot as well. Could add it there as well but it would break translations.
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.
Translations would continue work — translation team applies "fuzzy" label to strings that change only 1-2 characters, and old translation would continue being used. You can always confirm with them it's the case with this specific string too.
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.
Ah, didn't know that, thanks!
|
||
Jetpack_Subscribe_Overlay::init(); | ||
|
||
add_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.
Can you explain why we need this 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.
Good question! I would also like to know 😄
This is also copy&paste from the Subscribe Modal:
Lines 231 to 236 in 9393263
add_action( | |
'rest_api_switched_to_blog', | |
function () { | |
Jetpack_Subscribe_Modal::init(); | |
} | |
); |
@lezama do you remember why we need it for the modal? According to the git blame, you added it there.
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 noticed the Jetpack_Subscription_Modal_On_Comment
has it as well.
Lines 195 to 200 in f4c3844
add_action( | |
'rest_api_switched_to_blog', | |
function () { | |
Jetpack_Subscription_Modal_On_Comment::init(); | |
} | |
); |
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.
@lezama do you remember why we need it for the modal? According to the git blame, you added it there.
My guess is that it was there while we had the feature flag: https://github.com/Automattic/jetpack/blame/77189e0a87b3418f2d4d791c1ab83ac7b3f1d4f5/projects/plugins/jetpack/modules/subscriptions/subscribe-modal/class-jetpack-subscribe-modal.php
but if we are always calling Jetpack_Subscribe_Overlay::init();
it is probably not needed 🤔
projects/plugins/jetpack/modules/subscriptions/subscribe-overlay/subscribe-overlay.js
Show resolved
Hide resolved
} | ||
|
||
function setOverlayDismissedCookie() { | ||
// Expires in 7 days |
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.
❤️
add_action( 'wp_footer', array( $this, 'add_subscribe_overlay_to_frontend' ) ); | ||
} | ||
|
||
add_filter( 'get_block_template', array( $this, 'get_block_template_filter' ), 10, 3 ); |
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.
shouldn't this be under the feature flag too?
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 think it's safe to leave it as it is. It allows the user to edit an overlay template if he has a link but there is no link to it anywhere.
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.
Code looks good, we can continue iterate and improve behind feature flag.
Looks like there are some build failures for files, so needs looking into.
|
Fixed! |
Part of #36208
Proposed changes:
This is the first iteration of the Welcome Overlay feature (p1HpG7-so7-p2).
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.php
file:Known issues
There is no way of changing the overlay background color. I'll try to fix in in a follow-up PR.
Other information:
Jetpack product discussion
Does this pull request change what data or activity we track or use?
Testing instructions:
/wp-admin/site-editor.php?postType=wp_template_part&postId=visualblocks%2F%2Fjetpack-subscribe-overlay&canvas=edit
, edit the overlay content and make sure the changes are visible on the frontend (you'll need to delete thejetpack_post_subscribe_overlay_dismissed
cookie to render the overlay again)