-
Notifications
You must be signed in to change notification settings - Fork 796
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
Newsletters: Enhance Subscription Modal Display Logic #38079
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. |
Intersection observer API is now has good enough support, we should use that instead; it's more performant and simpler. I'm thinking if just relying on full page height works well in all cases, because sometimes sites have a lot of footer content too. 🤔 Some ideas: |
@lezama when Cris is back next week, can you work with her to find good trigger point for these modals? |
132c762
to
119e2bb
Compare
I tried using the Intersection Observer API without success. Although it’s great for detecting when two elements intersect, it doesn’t seem straightforward for our use case. Since a post occupies the entire viewport, it always intersects at the same proportion. |
I’ve just pushed a simplification. I believe it’s ready for testing. cc @enejb |
it now uses .entry-content or fallbacks to document |
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.
Still need to test this but I think we should rename the filter since the numbers might not make sense for users that area currently using them.
}; | ||
function onScroll() { | ||
if ( ! hasLoaded ) { | ||
requestAnimationFrame( () => { |
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 call on the requestAnimationFrame
@@ -64,17 +64,35 @@ public function enqueue_assets() { | |||
wp_enqueue_script( 'subscribe-modal-js', plugins_url( 'subscribe-modal.js', __FILE__ ), array( 'wp-dom-ready' ), JETPACK__VERSION, true ); | |||
|
|||
/** | |||
* Filter how many milliseconds a user must scroll for until the Subscribe Modal appears. | |||
* Filter how many milliseconds until the Subscribe Modal appears. | |||
* | |||
* @module subscriptions | |||
* | |||
* @since 13.4 | |||
* | |||
* @param int 300 Time in milliseconds for the Subscribe Modal to appear upon scrolling. |
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.
We should update this default comment since it changed.
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.
Should we introduce a slightly different filter name since this one might be in use already.
cc: @Aurorum for awareness and feedback, since you introduced the original 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.
No strong views from me! It was added to allow users to customise the default value for the load time; if the default value still changes, I don’t think that’s a big deal.
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.
We should update this default comment since it changed.
done!
@@ -57,6 +75,7 @@ domReady( function () { | |||
hasLoaded = true; | |||
setModalDismissedCookie(); | |||
window.addEventListener( 'keydown', closeModalOnEscapeKeydown ); | |||
window.removeEventListener( 'scroll', onScroll ); |
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 call on removing this event listener.
I tested the change on mobile and desktop and it works really nice! |
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.
Looks good!
Fixes p1719421326241939/1719262060.786719-slack-C02NQ4HMJKV
Proposed changes:
Other information:
Jetpack product discussion
Does this pull request change what data or activity we track or use?
Testing instructions: