-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
Publicize: Adding to Gutenberg Extensibility for Publicize Support #5795
Conversation
@gziolo , I wanted to get this PR in front of you. Please let me know if there are other specific people that should keep an eye on this. I haven't yet added the post-publish functionality that we discussed. I plan to try it out over this week. |
For the UI extensibility, I think we should follow the same pattern established for a sidebar: const { Fragment } = wp.element;
const { PrePublish } = wp.editPost.__experimental;
const { registerPlugin } = wp.plugins;
const Component = () => (
<Fragment>
<PrePublish name="jetpack">
{ /* Render Jetpack's pre-publish settings here */ }
</PrePublish>
</Fragment>
);
registerPlugin( 'plugin-name', {
render: Component,
} ); For pointers on how to implement this you could take a look at the Sidebar PR: #5430. Maybe there should also be a Related: #5767. |
Is there anything specific that should be executed on the frontend that can't be performed on the server? In this case you are adding a hook after I think @aduth had some good idea how it all can be handled using only effects and data store. |
"Must" is a strong word, implying there is no other alternative. Based on Automattic/jetpack#9144 , I'm assuming this is so that the post can be flagged as not being shared for Publicize. Why can't this occur earlier in the editor lifecycle? Why can't this be the default to not publicize by default? Why can't Jetpack hook into server API save filters? It seems to promote a poor user experience to block editor saving so that a plugin's own network request can go through. And if it's not blocking, which is the case as implemented here, there's a very likely risk for race conditions between the two requests. |
Hey, @aduth. Very valid points. The behavior I'm looking at implementing here is definitely not a hard requirement overall, just a piece of what's needed to support the current strategy in Automattic/jetpack#9144. I edited the post with softer wording here to avoid miscommunication. We had some discussion on the original Jetpack issue about this approach: Automattic/jetpack#9039. I'm not particularly happy with the current solution of generating the request right before publish. As far as I know, there's not currently any reliable way to differentiate a post being published from Gutenberg vs. a post being published from other sources (classic editor, mobile, API, etc.). Having this action occur earlier in the editor lifecycle may be the best approach if we don't want to hold the "Publish" action while it finishes. The downside of this is it increases the chance that a post is "flagged" and then is later published through other means. If a post is flagged, and then a user publishes elsewhere, then it will not be publicized. A definite corner case, but it'll happen sometimes. I wonder how much we really have to worry about it... |
Yeah, to me, the more concerning thing is that we feel the need to introduce a different behavior for Gutenberg posts vs. other sources. Another consideration per your point that there's no way to distinguish a Gutenberg point: Maybe a plugin could add such a distinguishing marker? There's a few steps of post preparation that occur when the editor loads. Maybe one of those could be adding some (temporary?) meta value marking the post as being edited in Gutenberg. I'm not sure this ought to be a core behavior, for the reason mentioned above (distinguishing Gutenberg should at the very least not be a "blessed" behavior). Even if there were to be some pre-publish hook, I don't think an action is necessarily the best fit. As mentioned previously it's prone to race conditions. I imagine it would be more an array of promises that a plugin could filter into, and is passed through to |
@gziolo, the current approach in Automattic/jetpack#9144 needs to have a post flagged before it is published. That PR provides a REST endpoint to do just that. The crux of what I need is the ability to differentiate between a post being published from Gutenberg and post being published from other sources. |
This is definitely doable, but I don't think 100% foolproof. A post could be flagged right when Gutenberg opens up and (if the flag is set up to expire after a set time) it could even be refreshed periodically using the Heartbeat API. Unfortunately that still leaves open the possibility that a user opens the post in Gutenberg, quickly publishes via some other means, and then finds that the post has not been shared. I don't have a very good perspective on if that's too small of a corner case to worry about.
I wonder if there's any chance other plugins will have a similar need. This need is coming about because the design concept takes advantage of Gutenberg's post-publish panel to give the user control over an action that normally happens automatically upon publish. If Jetpack takes advantage of that flow, will there be others to follow, or is this really a once off?... Speaking from a perspective of only making this one feature work, it would be very nice to get a 'post_publish_available' key in the post request to trigger special back-end actions whenever Gutenberg publishes. I could totally avoid all the kludge of trying to slip another request in before publish occurs. The post-publish view does potentially introduce a new way of splitting up actions that might have happened automatically with publish. Maybe there would be justification for introducing this to allow for plugins to take advantage of the new flow while preserving backwards compatibility? I definitely don't presume to have a very good perspective on the overall architectural goals. 😃 I'm just brainstorming a way to make this work in a clean way. |
There are a number of plugins that check aspects of posts for missing elements and intervene during the publication process to prevent for example, the publication of a post with a duplicate title or missing featured image, Ideally these plugins could provide support for Gutenberg and to my mind you'd need to run different code depending on if Gutenberg is the editor or not. I think we need that level of granularity. |
The same code is used to process posts in both the classic editor and Gutenberg. Whatever works at the moment, it is going to work the same in Gutenberg. The fact that we are using REST API doesn't mean it works differently to whatever is now in the classic editor. If you prevent the post to be updated then you just need to make sure that API response contains the feedback that can be presented to the user in Gutenberg.
@pento recently added a shim to override
We might add something along those lines:
Having that exposed, you can use PHP hooks to add as many changes as required for the plugin to make such flow work. /cc @adamsilverstein |
@gziolo, That's a great idea. I just reverted the last commit and used your suggestion instead: |
Glad to hear it simplifies things. I would double check if that shim is loaded when Gutenberg plugin is enabled vs when the post is edited with Gutenberg. It might be the first option as far as I understood when talking to @pento. |
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 left a few early comments.
#### Usage | ||
|
||
```jsx | ||
<PluginPrePublishPanel> |
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 be Post not Pre 😃
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.
Fixed 😃
* @see /edit-post/README.md | ||
* | ||
* @file This files defines the PluginPostPublishPanel extension | ||
* @author ChrisShultz |
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 don't use @author
in the project.
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 to know. I'll kill it.
Should I be consulting any standards other than what's here? - https://make.wordpress.org/core/handbook/best-practices/inline-documentation-standards/javascript
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.
Fixed
*/ | ||
import { Slot, Fill } from '@wordpress/components'; | ||
|
||
/** |
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 need it to put it here if there are no deps.
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.
Thanks. I removed the unnecessary @see /edit-posts/README.md
* | ||
* @return {WPElement} Plugin sidebar fill. | ||
*/ | ||
function PluginPostPublishPanel( { children } ) { |
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 factory method to remove code duplication (similar to the new React Context AP)?:
const createSlotFill = ( slotName ) => {
function Component( { children } ) {
return (
<Fill name={ slotName } >
{ children }
</Fill>
);
}
Component.displayName = slotName;
Component.Slot = () => (
<Slot name={ slotName } />
);
return Component;
};
then in this file:
export default createSlotFill( 'PluginPostPublishPanel' );
/cc @aduth
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's a good idea. No sense duplicating code. I can only assume there will be the need for more slots as Gutenberg evolves.
I wonder where createSlotFill
should site. Potentially move it into a subfolder in the top level 'components' directory?
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 introducing this helper in another PR:
gutenberg/components/slot-fill/index.js
Lines 12 to 27 in eecc528
export function createSlotFill( name ) { | |
const Component = ( { children, ...props } ) => ( | |
<Fill name={ name } { ...props }> | |
{ children } | |
</Fill> | |
); | |
Component.displayName = name; | |
Component.Slot = ( { children, ...props } ) => ( | |
<Slot name={ name } { ...props }> | |
{ children } | |
</Slot> | |
); | |
return Component; | |
} |
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.
Awesome! I'll look at incorporating this.
@@ -11,6 +11,7 @@ import PostVisibility from '../post-visibility'; | |||
import PostVisibilityLabel from '../post-visibility/label'; | |||
import PostSchedule from '../post-schedule'; | |||
import PostScheduleLabel from '../post-schedule/label'; | |||
import PluginPrePublishPanel from '../../../edit-post/components/plugin-pre-publish-panel'; |
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.
editor
module shouldn't depend on edit-post
.
@youknowriad, should we move the PublishPanel to edit-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.
What's the high-level difference between which content is in edit-post
vs editor
?
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.
editor
is a generic module for building any editor layout
edit-post
is the layout built for editing posts/pages/cpts
we can imagine edit-p2
edit-template
...
A simple solution here would be to use the children
prop.
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.
editor is a generic module for building any editor layout
edit-post is the layout built for editing posts/pages/cpts
we can imagine edit-p2 edit-template...
Aha, I see. It makes sense to me now why we can't have editor
depending on edit-post
.
A simple solution here would be to use the children prop.
Are you suggesting including PluginPrePublishPanel
in the edit-post
layout (/edit-post/component/layout/index.js). Maybe something like the following? --
{ publishSidebarOpened && (
<PostPublishPanel
onClose={ closePublishSidebar }
forceIsDirty={ hasActiveMetaboxes }
forceIsSaving={ isSaving }
>
<PluginPrePublishPanel.Slot/>
</PostPublishPanel>
) }
And then of course, passing it on the way down so it gets included in the right place in the editor
layout?
That would keep editor
as generic as possible.
That being said, I could see the advantage of including the PluginPrePublishPanel
Slot in editor
. I'm guessing this extensibility would be useful for any editor
implementation.
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 being said, I could see the advantage of including the PluginPrePublishPanel Slot in editor. I'm guessing this extensibility would be useful for any editor implementation.
I don't think we should force an extensibility pattern like this to all editors. Saving a template is not the same as saving a post. I'd prefer the former (like the example you shown)
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, children
seems like the way to go 👍
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.
@gziolo, I incorporated the new createSlotFill! Thanks!
I also made the change to remove the edit-post
dependency in editor
. I couldn't use strictly children
since the top level editor
component <PostPublishPanel>
contains both the 'pre' and the 'post' components. I created prePublishExtension
and postPublishExtension
for this purpose instead to route the plugin slots down to right places.
@c-shultz, there went something wrong with merging your changes with the |
Shoot. Thanks for pointing that out, @gziolo. I'm going to see if I can get that cleaned up. I need to get my brain out of SVN mode! Edit: Got it cleaned up now. |
b190ecc
to
ad1dd52
Compare
To expand on this, sometimes we want to intervene before the publish button is even clicked, for example, imagine I don't want posts to be publishable until the author has added a featured image. So I want to disable the publish button and add a message above it that says 'add a featured image to publish'. once the featured image is added, the publish button should be enabled and the message should go away. Similarly, published posts would disable 'update' if you tried to remove their featured image. Is this type of extensibility currently possible, can I do this in my plugin or theme?
Adding a unique header for Gutenberg requests is a great idea. |
ca2c0c3
to
f172f42
Compare
@adamsilverstein Viewed another way, this is not an intervention at publish time, this is an ongoing requirement of the post's state which is satisfied at a known point in time: when the featured image is assigned. It's not currently possible to achieve to my knowledge, though I would imagine it relating to the behavior of the |
f172f42
to
ae5b05c
Compare
@aduth yea, being able to extend |
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 think of any useful unit testing that could be done here with the current content?
Slot & Fill are covered with tests, so it might be not necessary to add new tests with what we have at the moment. It seems to be fine as it is.
@@ -83,6 +84,7 @@ class PostPublishPanelPostpublish extends Component { | |||
</ClipboardButton> | |||
</div> | |||
</PanelBody> | |||
{ <PluginPostPublishPanel.Slot /> } |
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.
Curly braces ({
+ }
) are obsolete.
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.
✔️
@@ -29,6 +30,7 @@ function PostPublishPanelPrepublish() { | |||
] }> | |||
<PostSchedule /> | |||
</PanelBody> | |||
{ <PluginPrePublishPanel.Slot /> } |
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 same comment about curly braces applies.
@@ -11,6 +11,7 @@ import PostVisibility from '../post-visibility'; | |||
import PostVisibilityLabel from '../post-visibility/label'; | |||
import PostSchedule from '../post-schedule'; | |||
import PostScheduleLabel from '../post-schedule/label'; | |||
import PluginPrePublishPanel from '../../../edit-post/components/plugin-pre-publish-panel'; |
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, children
seems like the way to go 👍
0258b49
to
946ee4d
Compare
Creates a 'prePublish' action hook that is triggered right before a post is published.
This reverts commit 81696afec2bd3c37e78d944b73c7978b06431ad2. Using action hooks and an extra request was deemed too much kludge just to differentiate publishing from Gutenberg. Another iteration is incoming...
As a method for differentiating requests from Gutenberg editor, a header keyed 'X-WP-Source' is being added and set to 'Gutenberg'.
Adding slots for pre-publish sidebar and post-publish sidebar to be used with `registerPlugin` for plugins to add content to the bottom of these sidebars. The current plan is to use the post-publish sidebar will be used for Jetpack's Publicize feature.
Cleaning up some leading and trailing whitespace issues.
Fixing typos and clearing out unecessary DocBlock entries.
Removing obsolete curly brackets around PluginPostPublishPanel.Slot and PluginPrePublishPanel.Slot components.
Now passing pre/post publish plugin slot using new prop and children prop instead of referencing it directly from within editor. props @youknowriad
Replacing repetitious code with newly created createSlotFill factory function for PluginPrePublishPanel and PluginPostPublishPanel slots. props @gziolo
946ee4d
to
658fdc4
Compare
lib/compat.php
Outdated
@@ -170,6 +170,7 @@ function gutenberg_shim_api_request_emulate_http( $scripts ) { | |||
options.headers = {}; | |||
} | |||
options.headers['X-HTTP-Method-Override'] = options.method; | |||
options.headers['X-WP-Source'] = 'Gutenberg'; |
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 do we need this? Is this temporary? We also started removing "Gutenberg" code name from the source code in general.
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.
Short answer:
There's no need for this anymore based on the current approach in in Automattic/jetpack#9144.
Longer answer:
The original need came about because Publicize was being included in the post-publish panel so the post would be shared manually by the user after the post was already published. The existing behavior of Publicize is to automatically share published posts to all connected social accounts. In a previous iteration of Automattic/jetpack#9144, the Publicize back-end used X-WP-Source
to detect posts coming from Gutenberg so they would not be immediately shared. That being said, the design direction for Publicize integration changed to be pre-publish based, so this is now unused.
Work continues in #6798. |
Fixes #3264
Description
To support the current Publicize integration strategy per Jetpack PR Automattic/jetpack#9144, I'm proposing Gutenberg provide extensibility in a couple areas:
Provide a hook that is triggered just before publish that can trigger plugin callbacksProgress so far
The progress to date in this PR adds a 'prePublish' action hook that is triggered just before a post is published.X-WP-Source='Gutenberg'
to request header to differentiate post updates coming from Gutenberg.PluginPrePublishPanel
andPluginPostPublishPanel
to for plugins to add content to pre-publish sidebar or post-publish sidebar respectively. Here's a preview of some test content ("Hello world from post publish." inPluginPostPublishPanel
:TODO items:
Iterate 'prePublish' iteration to allow for asynchronous callsAdd post-publish plugin extendability using registerPluginFirst iteration doneTesting
'prePublish' action can be tested in browser console, usingwp.hooks.addAction( 'prePublish', 'publicize', function(){alert("Hello World");});
which should trigger alert when post is published.X-WP-Source='Gutenberg'
header in place, Gutenberg requests can be picked up on the server side by checking'Gutenberg' === $_SERVER['HTTP_X_WP_SOURCE']
-Using the experimental
Plugin<Pre/Post>PublishPanel
s:Checklist: