-
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
Add a GlobalStylesScreenRootSlot slot in Global Styles root screen #49136
base: trunk
Are you sure you want to change the base?
Add a GlobalStylesScreenRootSlot slot in Global Styles root screen #49136
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
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 for your work here, that's a great start.
Like mentioned here #48068 (comment) I think we should start by protecting this API with the Gutenberg plugin check, can we introduce this check.
Also, there's a couple things that we should consider:
- Adding an e2e test for this extensibility API potentially.
- Documentation: Not entirely sure how we're documenting the editSite slots in the block editor handbook but we should be consistent. It doesn't have to be in this PR since this is still experimental but let's make sure to have a todo item in the issue at least.
@@ -52,6 +71,8 @@ function ScreenRoot() { | |||
|
|||
return ( | |||
<Card size="small" className="edit-site-global-styles-screen-root"> | |||
<ScreenRootSlot /> |
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 a too prominent place to add a slot and plugins will compete too much for place. I think the preview and variations should stay at the top. Let's move this slot after the "browse styles" button
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'd echo Riad on this one. After browse styles
seems like a good position.
@@ -27,6 +29,23 @@ import ContextMenu from './context-menu'; | |||
import StylesPreview from './preview'; | |||
import { unlock } from '../../private-apis'; | |||
|
|||
const { Slot: GlobalStylesScreenRootSlot } = createSlotFill( |
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 be exposing the API in edit-site/src/index like we do for all the slots?
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.
Thank you for the PR @BogdanUngureanu, this looks good in general!
Besides Riad's notes, I'd suggest to move the slot in its own file.
Hi @BogdanUngureanu This seems like a promising start to get this into the Gutenberg plugin for testing. Would you be available to adjust your PR to incorporate the comments you received from @ntsekouras and @youknowriad |
What?
Resolves: #48068
Add a
GlobalStylesScreenRootSlot
slot in the Global Styles root screen.Why?
In order to allow integrators a way to add custom elements in the Global Styles Screen Root, a new slot is added at the top of the section.
How?
It creates a new slot that's called at the top of the GlobalStylesScreenRoot component.
Testing Instructions
It doesn't change the current behavior.
Open the Global Styles in site editor and make sure nothing changes.
Testing Instructions for Keyboard