-
Notifications
You must be signed in to change notification settings - Fork 813
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
Email preview: fix sending preview if content in the editor is different from latest in the database #34419
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. |
// Save post revision so that we send what they see in the editor, and not what previous draft/revision might've saved | ||
// Introduced at GB 16.3 at https://github.com/WordPress/gutenberg/pull/44971 | ||
if ( typeof __unstableSaveForPreview === 'function' ) { | ||
await __unstableSaveForPreview(); |
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.
nice!
tracks.recordEvent( 'jetpack_send_email_preview', { | ||
post_id: postId, | ||
} ); | ||
|
||
setEmailSending( true ); | ||
|
||
// Save post revision so that we send what they see in the editor, and not what previous draft/revision might've saved | ||
// Introduced at GB 16.3 at https://github.com/WordPress/gutenberg/pull/44971 |
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.
@Automattic/jetpack-crew in case you had advice on how to handle Gutenberg functions and backwards compatibility with older WPs in Jetpack; this isn't really issue for .com since we run the latest.
Method introduced in WordPress/gutenberg#44971
It's here, and it's contents are stable so in theory I could just copy all that code here instead.
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 needs to support back to the version of the Gutenberg packages that is bundled in the previous WordPress release, which is currently 6.3.
I guess the case here is that 6.3 (and maybe 6.4 too?) doesn't have any version of the function at all, and you're wanting to copy-paste the implementation into Jetpack so the change you're making here still happens in 6.3? Be sure to include a comment indicating that the back-compat code can be removed when we drop support for the old version. Something along the lines of // @todo Remove the `if` check and `else` branch once WP 6.4 is the minimum supported version
is fine, you don't have to get too fancy. Just be sure the comment clearly mentions an appropriate WordPress version so we can find it easily via a grep when we're making the PRs like #34210.
Or if it's ok that WP 6.3 just doesn't do whatever the new function does, that's fine with us too. Again though a comment mentioning the WP version when the if
check can be removed would be useful.
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.
Done!
projects/plugins/jetpack/extensions/blocks/subscriptions/email-preview.js
Outdated
Show resolved
Hide resolved
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.
Comment looks good to me now. Haven't reviewed the code itself, but someone else did earlier.
Proposed changes:
Other information:
Jetpack product discussion
Does this pull request change what data or activity we track or use?
Testing instructions: