-
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
Extensibility: Add publish panels support for plugins #6798
Conversation
@@ -84,6 +86,8 @@ function Layout( { | |||
onClose={ closePublishSidebar } | |||
forceIsDirty={ hasActiveMetaboxes } | |||
forceIsSaving={ isSaving } | |||
renderPrePublishExtension={ () => <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.
Maybe here we can just have a renderPublishExtension prop, and leave it up to the component to query state and check if we are in pre-publish or post-publish and just render when we are in the desired state ( or both ).
Later we could even change for a more generic sidebar extension mechanism where we just extend all sidebars the component is responsible to query the state to discover if it should render something in a given state or not (e.g: if it is pre-publish) if it is the block sidebar etc...
This would even allow a plugin to extend the sidebars of other plugins :)
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.
Those two are mutually exclusive. You are either in pre-publish screen or post-publish screen. They are passed down to two different subcomponents. This is limited to publish sidebar only. Can you elaborate on your idea with some code examples? I must admit that I don't quite see how this part could relate to the plugin sidebar.
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.
Hi @gziolo here is a sample of the generic API for extending sidebars that I was referring: #6899.
Unfortunately, it ended up not being possible to use it in this case, because pre and post-publish state are not available globally for plugins to query. It uses local state.
But the initial idea was to render <SidebarExtender.Slot /> both pre and post-publish panels.
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.
Right, this component has its own internal state which controls the logic for rendering individual panels. I will check your PR. Thanks for opening 👍
I tested the PrePublish with the follwing code block and it worked well:
Regarding post publish it also worked well but only when post publish panel appears e.g.: scheduled posts. I think we may need to add a mechanism that allows plugins to change the rules of when post publish appears so they can make the post publish appear even in more common publishing situations. |
Yes, this was confusing also for me. I don't know the details why it works how it works, but it would be nice to be able to change it. |
It's a regression. You should always see the post publish panel. It gets immediately closed when publish a post in 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.
Leaving a design review 👍
18bc41e
to
efa8098
Compare
@jorgefilipecosta it was fixed with #6822. You should always see both panels when publishing posts. |
|
||
```jsx | ||
const { __ } = wp.i18n; | ||
const { PluginPrePublishPanel } = wp.editPost; |
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.
Shouldn't these APIs just be EditorPrePublishPanel
instead of Plugin...?
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 prefixed all other occurrences with Plugin
in the past, this just keeps the convention.
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.
See the list of components in https://github.com/WordPress/gutenberg/blob/efa8098979a45127d7ac74ec0df600496c19bdac/edit-post/README.md
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, but I'm not sure it makes a lot of sense for every piece. :) PluginSidebar
and the plugin menu item make sense because their entire purpose is plugin based.
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 not attached to this, we can update.
What about PluginPostStatustInfo
, should we also rename to EditorPostStatusInfo
?
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 did some more research. We already have PostPublishPanel
. See:
https://github.com/WordPress/gutenberg/blob/master/editor/components/post-publish-panel/index.js#L66
which uses editor-post-publish-panel
class name. It might be a bit confusing. Maybe we should keep this Plugin
prefix to make it clear that this is meant to be used by plugins.
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, that's fine, was just a comment because it felt a bit weird to go with Plugin as a generic prefix across the board.
@@ -94,3 +94,4 @@ This list is manually curated to include valuable contributions by volunteers th | |||
| @Cloud887 | | |||
| @hblackett | | |||
| @vishalkakadiya | | |||
| @c-shultz | |
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.
does this belong 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.
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.
Perfect.
All good here? |
Yes, let's proceed 👍 |
@@ -84,6 +86,8 @@ function Layout( { | |||
onClose={ closePublishSidebar } | |||
forceIsDirty={ hasActiveMetaboxes } | |||
forceIsSaving={ isSaving } | |||
renderPrePublishExtension={ () => <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.
This will cause render reconciliation more than necessary because it's creating a new function each render.
Why not hoist to a top-level variable? Or could renderPrePublishExtension
be interpreted as a component, so we could pass the PluginPrePublishPanel.Slot
reference directly instead of a 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.
Interesting, I would also think we would cause render reconciliation more than necessary because we are passing new props on each rerender, but the react samples for the Render Props pattern use something similar https://reactjs.org/docs/render-props.html, they are also creating inline functions and passing them as props. I see this practice being used widely. I'm curious if react has some an optimization for the inline functions rendering React 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.
We probably can pass those slots as components in this case. Opening PR soon.
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: #6938.
Description
Fixes #3264.
Continues work started by @c-shultz in #5795. To make it easier to proceed I moved those changes to the upstream repository. Sorry, for losing commits history @c-shultz and thanks for your initial work which made this change very easy.
How has this been tested?
Code to paste into your browser: https://gist.github.com/gziolo/fd8141f2cde7fcb1b513c09eea45d27f.
Screenshots
Pre-publish
Post-publish
Desktop:
Mobile:
Types of changes
New feature (non-breaking change which adds functionality).
Checklist: