-
Notifications
You must be signed in to change notification settings - Fork 809
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
Hide sharing buttons in Blog Posts block in the editor #25346
Conversation
Caution: This PR has changes that must be merged to WordPress.com |
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 (other than "Required review") appearing at the bottom of this PR are passing or skipped. Jetpack plugin:
|
@@ -942,6 +942,11 @@ function sharing_display( $text = '', $echo = false ) { | |||
return $text; | |||
} | |||
|
|||
// Prevent from rendering sharing buttons in block which is fetched from REST endpoint by editor | |||
if ( defined( 'REST_REQUEST' ) && REST_REQUEST ) { |
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.
@coder-karen @jeherve Just to confirm, we'd like to always remove the share buttons in REST API responses?
If we wanted to sometimes include share buttons in the REST API response, an alternative approach might be:
- Only include share buttons in the REST API response when share buttons are enabled for the post type.
- Don't ever include share buttons in the REST API response when it's a dynamic block request.
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 should think we'd not want it in the REST API response, but if we did it would be when they're enabled for the post type.
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.
Given that the injected sharing buttons require enqueued JS to function further down in the chain, I'm not sure why anyone would want the button content to show up in REST API responses 🤔
This tested well for me, and I think it'd be safe to ship and observe for any issue requests that pop up.
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.
When the editor requests this block, it appends the context
GET param set to the edit
value. I could include it in the condition to address @danielbachhuber concerns. What do you think?
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.
That could work, assuming the main intention is to hide the sharing buttons in the editor. That would tie in with the original linked issue.
Perhaps determining which REST API conditions to include sharing buttons in could be for a separate PR, with this one just removing them from the editor. That said I'll be releasing 11.2 in a few hours so I'm not sure if any of this should be a blocker. Given that the issue itself (share button output in the editor) has been around for quite some time it may be better that I don't try to rush this in to 11.2, then we can make sure all cases are covered.
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.
Or I can just merge as is, like @samiff suggested we can look out for any issues as is, though we could perhaps refine what we do and don't want to include in a further PR. I'll do this then unless anything comes up in the next few hours.
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.
@coder-karen I've just added that additional check and retested it - it still works as expected.
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.
Yes it does. I'll merge this as is, then I think we can follow up in another PR should we need to be stricter with the REST API conditions.
Great news! One last step: head over to your WordPress.com diff, D85150-code, and deploy it. Thank you! |
r249967-wpcom |
Cherry-picked to release branch in b3b1455 |
Fixes #25329
Changes proposed in this Pull Request:
Other information:
Have you written new tests for your changes, if applicable?Jetpack product discussion
N/A
Does this pull request change what data or activity we track or use?
No
Testing instructions:
Alternatively, use an Atomic site with the Jetpack Beta plugin and use this PR branch there.