-
Notifications
You must be signed in to change notification settings - Fork 799
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
Add Gutenberg Publicize Support #9144
Conversation
public function __construct( $publicize ) { | ||
$this->publicize = $publicize; | ||
|
||
add_action( 'rest_api_init', function () { |
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.
Not all version of PHP that we support have anonymous functions available.
* | ||
* @since 5.9.1 | ||
*/ | ||
class Async_Publicize { |
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.
Lets prefix this with 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.
So, just to be clear, "Jetpack_Async_Publicize"?
* | ||
* @return None | ||
*/ | ||
postPublishPublicizePost = function ( post_id, message ) { |
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.
postPublishPublicizePost
- Can we find a better method name here?
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.
Latest iteration goes with "sharePost"
Instead of having another endpoint that stops the posts from publicizing can we hook into the current set_post_flags method somehow and set_post_flags so that we don't publicize the post. We currently have an endpoint already for .com sites that lets us publicize posts at anytime. It is a feature that we expose to our premium customers but we could reuse it to make it work for Gutenberg posts that have not been published. This way we can simplify things a bit. |
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.
A few comments about coding style and enqueuing resources.
add_action( 'jetpack_published_post', array( $this, 'post_publish_cleanup' ), 10, 2 ); | ||
|
||
// Do post edit page specific setup. | ||
add_action( 'admin_enqueue_scripts', array( $this, 'post_page_enqueue' ) ); |
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 believe you would need to use the enqueue_block_editor_assets
hook here, so your script only gets enqueued in the Gutenberg editor:
https://github.com/WordPress/gutenberg/tree/v2.4.0/blocks#getting-started
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! I wasn't aware of enqueue_block_editor_assets
*/ | ||
public function post_page_enqueue( $hook_suffix ) { | ||
if ( ( 'post.php' === $hook_suffix || 'post-new.php' === $hook_suffix ) ) { | ||
wp_enqueue_script( 'async_publicize_js', plugins_url( 'assets/async-publicize.js', __FILE__ ), array( 'jquery' ) ); |
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.
You may want to add a dependency for the wp-blocks
and wp-element
script handles as well:
https://github.com/WordPress/gutenberg/tree/v2.4.0/blocks#getting-started
} | ||
|
||
// If post does not exist. | ||
if ( get_post_status( $post_id ) === false ) { |
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.
You'll want to use Yoda conditons, as per the WP coding standards:
https://make.wordpress.org/core/handbook/best-practices/coding-standards/php/#yoda-conditions
$post_id = $request['post_id']; | ||
|
||
if ( ! current_user_can( 'publish_post', $post_id ) ) { | ||
return 'Current user cannot publish this post'; |
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.
It may be nice to make those strings translatable.
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.
Noted. If the endpoints stick around after the next iteration of this, I also intend to put the responses in a WP_REST_Response
instance.
Simple front end form now added to post-publish view in Gutenberg: I need to hook this together with the function call that actually shares the post. To test, this PR must be paired with the Gutenberg PR I've been working on: WordPress/gutenberg#5795 |
Thanks all for the feedback! I will be addressing these issues when I'm able. I've been focusing on the Gutenberg extensibility side of this project for now. |
Latest commit is a major iteration on the UI:
The UX is still rough:
UI so far: CC: @MichaelArestad (not looking as good/functional as your design yet, but I think it can get there 😃) |
Props @haszari for the tip on hooking into webpack.config.js! |
I've been asked to go with the pre-publish flow instead of post-publish as a first iteration. The latest commit makes this update. Using pre-publish also enables quite a bit more code re-use on the backend. |
28db1b0
to
1f49bc2
Compare
|
7f1bc46
to
177cd6c
Compare
Publicize form appears but no longer has any effect after rebasing. The form appears but checking/unchecking and setting the message are all ignored. WordPress/gutenberg#5971 moved post data to HTTP payload as JSON instead of using $_POST header, which broke publicize.php parsing of $_POST. publicize.php will likely need modification to parse JSON value instead. |
class PublicizeForm extends Component { | ||
constructor( props ) { | ||
super( props ); | ||
let { connections } = this.props; |
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.
Why is this a let
, not a const
?
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'm pretty sure it was leftover from an in-progress version of that code. It clearly should be const
now though! This is now fixed.
Update on my earlier comment:
This is now fixed |
d2e995e
to
ff91645
Compare
I know styling still needs work so I'll hold off on those comments for now. I will say, what you have is a pretty dang good start. :) Testing feedback:
Nice work. |
Hey, @MichaelArestad! Thanks again for checking things out!
This is definitely doable! For my own knowledge, why is this preferable over a popup that saves the user from reloading the editor?
How do you envision the redirect back to the post functioning? Maybe something like a "Go back to your post" link added to the connection settings page?
👍 Makes perfect sense. I'll make that change.
The refresh event is triggered by the user closing the "Settings" popup. In the event that they didn't close it, but just switched back, they wouldn't get the update. I wanted to give the user a backup option, but there may be alternatives to showing the link... If we don't go with a popup though, this becomes irrelevant. There might still be a case for showing it only in the event that there's a problem with the connections. I'll think on that...
Yeah, definitely. I also really like your design that shows the count in a footer attached to the textarea. I'll be planning on eventually matching that as well. |
Adding simple Readme to explain how Publicize flags posts to be shared not shared by WP.com.
Expose helper method done_sharing_post as a public method to support direct unit testing (and planned rework).
Adding test coverage for: - done_sharing_post: testing flagged and published posts - get_services_connected: testing service list - get_filtered_connection_data: testing simple connection data with no filtering
Adding unit tests for three existing filters in Publicize: - 'wpas_submit_post?' - 'publicize_checkbox_global_default' - 'publicize_checkbox_default' These filters were previously difficult to test since they were being applied within HTML generation code. Now that HTML generation is separated from the connection retrieving/filtering, it is a good time to add unit tests.
ui.php changes and associated test cases are being reverted out of this branch. For clarity/organization, these changes are going to be in try/publicize-decouple-ui instead.
The connection data access method was moved to the separate branch ('try/publicize-decouple-ui') This branch places the connection helper methods in a different class, so the calls to these methods needed to be updated. The 'disabled' field for the connections also changed from a string to a boolean so the frontend processing is updated as well.
Connection list request was an AJAX endpoint. This change converts this to a REST endpoint instead: 'publicize/posts/<post_id>/connections'
Adding one simple unit test for the Gutenberg Publicize settings button. The test is not very useful on its own, but proves out a path to creating tests from the modules subdirectory structure. Any .js file in 'modules' direction prefixed with 'test-' will be run as test. These tests are run with 'yarn test-gui'. More tests to come.
Minor whitespace cleanup: - missing space - Extra newline at the end of file props @ehg
Removal of unnecessary href props @ehg
Ambiguous boolean parameter being replaced with named value in an 'options' parameter in connectionChange method. props @ehg
'...' becomes '…' props @ehg
PublicizeForm contained both the main component and the 'wrapper' withSelect/withDispatch setup to use editor store. This changes separate this so PublicizeForm contains wrapper code and component is moved to PublicizeFormUnwrapped so it can be tested separately.
Adding new test file for PublicizeFormUnwrapped. Tests cover basic behavior of form such as displaying connections, alerting maximum string length, and disabling form if connections are disabled.
Class string for string length warning was manually generated. This commit uses classnames instead for cleaner and less-error prone code. props @ehg
Simplifying code by using 'some' utility method to check if any connections are not disabled. props @ehg
Replacing undesirable non-strict null comparison with _.isNil(). Both should return true if value being checked is either null or undefined. props @ehg
Previously, the current post ID was being retrieved from the DOM. This change uses withSelect to get it from the editor instead.
The connection verify request was still contained within a React component. This moves the request into a function in 'async-publicize-lib.jsx' and also updates the useless header in 'async-publicize-lib.jsx' to be less vague.
One concern with the flow for adding a connection is the fact that a popup is opened and the user may not realize this and my have a hard time getting back to the editor. This experimental change attempts to remedy this by putting a big ugly banner on the popup setting page that, when clicked, closes the pop up windows and bring the user back to the editor. Style will need some work if we decide to keep this approach.
f6ea372
to
d8a46b2
Compare
@oskosk, rebased and merged here to an Automattic repo branch: https://github.com/Automattic/jetpack/tree/add/publicize-gutenberg I'll look for a chance to sort out why the unit tests are failing here after rebase. |
Closing in favour of #9934 which is based on a branch local to this repo (instead of a fork of the repo). |
Superseded by #9934
Fixes #9039 (No Publicize form in Gutenberg)
Dependencies:
Changes proposed in this Pull Request:
Summary
Bringing Jetpack's Publicize feature to the Gutenberg editor! ---
Without this PR, Publicize invisibly shares all new Gutenberg posts to all connected social accounts.
Front-End Design Note
The design is based heavily on @MichaelArestad's post-publish design concept. The desired long-term direction for Publicize is to incorporate it in the post-publish panel (after the user publishes the post), but on this PR, as a first iteration, it is being placed in the pre-publish panel.
Architecture Notes
This PR touches a lot of files and there's several early commits that were taking the design in a different direction. In an effort to make review easier, I'll try to distill the list changes into the most important new content and describe how that's hooked into existing content:
modules\publicize\assets\gutenberg_client
directoryFill
UI is created on this PR (inpublicize-gutenberg-plugin.jsx
) and the pre-publishSlot
is created in Gutenberg PR #5795jetpack\modules\publicize\class-jetpack-publicize-gutenberg.php
does the setup for the new UI.post
endpoint to pass in Publicizetitle
andconnections
form options when saving post:wp_ajax_get_publicize_connections
to expose connection list so client can refresh connectionstry/publicize-decouple-ui
(PR Decouple Publicize connection list access from UI scripts #9466) to expose connection data access methodsTesting instructions:
git checkout add/gutenberg-publicize && git merge try/publicize-decouple-ui --no-commit