-
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
Introduce new PluginTemplateSettingPanel slot #50257
Conversation
Size Change: +152 B (0%) Total Size: 1.37 MB
ℹ️ View Unchanged
|
<PanelRow | ||
header={ __( 'Editing history' ) } | ||
className="edit-site-template-revisions" | ||
> | ||
<LastRevision /> | ||
</PanelRow> |
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 is new to this PR. Did it slip in accidentally from another PR or was it intentional? I didn't see it mentioned in the PR discussion so thought I'd ask :)
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.
Oh nvm, I see it was moved here from template-card/index.js
.
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.
Oh.. I forgot to comment about it, thanks! Yes, I just moved it outside of TemplateCard component because I think it shouldn't be a part of that component.
Always yes to reducing mismatch 😄 |
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.
Currently there is a mismatch between site and post editors.
I assume this is why we're introducing the new slot :)
The PluginDocumentSettingPanel
also handles rendering the PanelBody
wrapper. Do you know if this is something we also want to match here?
Personally, I like devs explicitly handling the rendering of panels, but we might want to mention it somewhere in the docs.
Should we add this slot in block-editor package in order to be reused when we edit a template?
Maybe not at the moment.
Similar slots are usually contained in the editors. Moving the PluginDocumentSettingPanel.Slot
into the block-editor
package and reusing it here might cause features meant for the post editor to leak into the templates.
packages/edit-site/src/components/plugin-template-setting-panel/index.js
Outdated
Show resolved
Hide resolved
packages/edit-site/src/components/plugin-template-setting-panel/index.js
Outdated
Show resolved
Hide resolved
Co-authored-by: George Mamadashvili <georgemamadashvili@gmail.com>
Thanks for reviewing George!
Yes.
I don't think so. Consumers should be responsible of adding and styling their components accordingly. |
Would be good to add a screenshot to this PR for future reference of what part of the UI it concerns. Should it also be marked for dev note? |
Hey, @ntsekouras, we are using We noticed that importing the Even with the fix that I shared above when the
I suggest moving the component to another path so that when the developer imports it, no side-effect runs. What do you think? |
@gigitux it seems you might be "bundling" the edit-site dependency which duplicates some JS packages, no? |
We use the package `@wordpress/dependency-extraction-webpack-plugin' to extract the @WordPress dependecies. (source code) I will investigate more, and I will let you know! |
Available in WP 6.3 Not yet documented outside of WordPress/gutenberg#50257
Hi there @ntsekouras ! We are still facing issues with the usage of In addition to the error @gigitux mentioned over here that is still ongoing, there's also a conflict with the core's navigation block recently uncovered: whenever you add the Navigation block to any post while having a component importing from
We have been trying to circumvent this in different ways. In order to isolate the changes to the relevant template we tried:
But no luck so far. Since we are following the recommended steps for adding information in the site editor’s Template sidebar, would it make sense to address this limitation on GB? Thanks in advance! |
Yeah, it seems we have more side effects if we add the I'm not an expert in the bundling tools, but it seems to me that the edit-site shouldn't be bundled when in post editor and secondly maybe displaying block information in this specific slot might not be the best. I'm not sure if we have more extension points in the block inspector sidebar(if we don't, we should), but it feels it should be a better fit there. The specific slot is for plugins in edit site and it display information under the template card, which would be more fitting for template and site editing related actions. I'll cc @gziolo and @jsnajdr if they have some input regarding the bundling. |
When a plugin script imports something from a package, like Woo Blocks' import { PluginTemplateSettingPanel } from '@wordpress/edit-site'; Then that package becomes the plugin script's dependency. Not only the package imports are turned into externalized references like Because The same problem will arise when a plugin is loaded into the A good solution for this is that the plugin makes sure that scripts that reference the Woo Blocks doesn't do this -- the offending Instead, Woo Blocks should build |
Thank you so much @jsnajdr and @ntsekouras for your prompt feedback and for sharing all these details! Inspired by your input, the following solution was proposed for this problem: woocommerce/woocommerce-blocks#10388 |
I just stumbled on this Slot, I think this should have been just the That said, there's potentially a path forward where the implementation of |
That's a good thought, although I'm not really sure how would we proceed and keep backwards compatibility, since PluginDocumentSettingPanel Besides the above, I observed now that at some point where we introduced the |
Using |
Hm.. I thought I tried that in code.. Let me try again 😄 |
As @Mamaduka pointed out registerPlugin is optional, but the issue here for me is: It's not clear for me what would be the best path forward. I'll have to think more about it, but any suggestions are welcome. |
Maybe the best path is just to soft deprecate this one. |
What?
This PR introduces a new slot in the Template Sidebar below the main information like the Template Card.
Should we add this slot in
block-editor
package in order to be reused when we edit a template? 🤔 This could apply toTemplateCard
, etc.. Currently there is a mismatch between site and post editors.Testing Instructions
Code example:
Render the above component somewhere in your code.
Example screenshot