-
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
Move Global Styles APIs to @wordpress/block-editor #47098
Move Global Styles APIs to @wordpress/block-editor #47098
Conversation
Size Change: +505 B (0%) Total Size: 1.33 MB
ℹ️ View Unchanged
|
The outstanding questions I'm thinking about are:
I am thinking no and have both. One is a higher level
I think no, since having both creates two sources of truth which is confusing. I'm not sure of the best API though. Do we simply add |
@oandregal @youknowriad: What are your thoughts on the proposed architecture here and my questions above? (Ignore the code in this PR which is rough.) While researching |
Flaky tests detected in ceb08f5. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/3955674679
|
Yeah, what you said. The root difference between the two is that block editor's useSettings inspects block instances, while edit site's useSetting inspects block types. I remember that there was discussion about this at some point, and people mentioned that it could be worth having a global styles package, to support use cases such as onboarding, for example. I don't have a strong opinion on being a standalone package or part of block-editor package. One of those makes sense.
Depending on whether global styles it's a standalone package or part of the block-editor, I guess the answer would be different? cc @jorgefilipecosta as I think he'd have thoughts on the topic as well. |
Is it worth creating a new package when |
Might be better to keep them separate for the reasons @oandregal mentions above. I do think that whatever we do here should be reflected in
Probably not 😅 I'd be happy for it to live in block-editor. |
I think one could use the other (the one with clientId can use the other one) under the hood and augment it with "instance specific" logic. But that might require a lot of work, so fine to post pone probably.
Not entirely certain, the global styles variations use-case might force us to reply "yes" here unless we find a way to do it with the latter.
I agree that there's some duplication here, not clear thought what's the best solution :) There's another area where there is duplication that we should ideally remove: the UI. The different block inspector controls and the different levels of the global styles UI are very similar and sometimes we have duplicated components. We've had a discussion on how to unify these. Probably a lot of work for an unclear outcome. The elephant in the room here is that if this folder moves into the block-editor package, it means a lot of new stable APIs to maintain for a piece of UI/behavior that still in flux (though stabilizing more and more). This might mean, it's better to start with a "bundled" |
That's a really compelling point! |
8f85b7d
to
5ac562d
Compare
I tried to split the global styles APIs into a new "bundled" Since they're tightly coupled, I moved the global styles APIs back to I left I renamed In a follow-up PR we can refactor So, to clarify, what this PR does is:
|
You could also argue that my suggestion here was rubbish and just avoid this dependency (like today) |
useGlobalStyle, | ||
} from './hooks'; | ||
export { useGlobalStylesOutput } from './use-global-styles-output'; | ||
export { GlobalStylesContext } from './context'; |
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.
Do we want to give the whole context object as an API or just the "provider" for external consumers.
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's a few places where we need to read the raw context value e.g. variations.
gutenberg/packages/edit-site/src/components/global-styles/screen-style-variations.js
Line 45 in 64c6912
const { base, user, setUserConfig } = useContext( GlobalStylesContext ); |
@@ -15,77 +15,3 @@ function MyComponent() { | |||
return <GlobalStylesUI />; |
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.
Any thoughts on the UI? can we share with block inspector UI at some point?
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 should be possible I think to create high level UI components that live in @wordpress/block-editor
and are used by both the block inspector and global styles. Ideally only the value
and onChange
differs. So basically what you propose in #37064. I note that since you opened that discussion we've come a long way in making the UIs look very similar so ideally now we can make the shared components be very high level (e.g. TypographyPanel
, DimensionsPanel
).
I'll make a note to explore this!
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.
Looks good to me, personally, I still have some uncertainties but this looks like a good start.
Btw, I'd love to be pinged on PRs when we'll use these APIs in other parts of the block editor.
Even if we didn't make
Yeah, me too 😄
Will keep you posted! |
What?
useSetting
inedit-site
→useGlobalSetting
inblock-editor
.useStyle
inedit-site
→useGlobalStyle
inblock-editor
.useGlobalStylesReset
,useGlobalStylesOutput
andGlobalStylesContext
inedit-site
toblock-editor
as is.lock()
s the above APIs withinwp.blockEditor.experiments
.global-styles
CPT remain in@wordpress/edit-site
.Why?
More and more there is a need for blocks to access Global Styles. For example we want to:
placeholder
) is if a control is left empty (context)#46894 and #34178 address this need by adding
useStyle
(to complimentuseSetting
) to@wordpress/block-editor
. It's awkward though because any implementation ofuseStyle
needsgetValueFromVariable
to make sense of CSS variables. This is an implementation detail of and not something that I think should be exposed from@wordpress/block-editor
.Instead we can move the high level Global Styles APIs to
@wordpress/block-editor
and make Global Styles a first class citizen of the block editor. This seems inevitable to me and to some extent has already begun. For example the block editor has a__experimentalFeatures
setting which containsmergedConfig.settings
and the block settings REST API has__experimentalStyles
which containsmergedConfig.styles
.Testing Instructions
There's no modified or new functionality here so just need to check for regressions.