-
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
Centralize client side global styles mappings #25056
Centralize client side global styles mappings #25056
Conversation
Size Change: -1 B Total Size: 1.2 MB
ℹ️ View Unchanged
|
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.
LGTM and makes perfect sense to have them in a single place instead of repeating them 👍
Tested, I couldn't find anything wrong
@@ -165,6 +165,7 @@ class EditorProvider extends Component { | |||
'__experimentalEnableFullSiteEditing', | |||
'__experimentalEnableFullSiteEditingDemo', | |||
'__experimentalFeatures', | |||
'__experimentalGlobalStylesMapping', |
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 doesn't really make sense to me, it's not a setting. If I have a paragraph block (static block) in any gutenberg instance without any mapping defined, I expect the "padding" control to work properly.
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 @youknowriad, I agree this is not exactly a setting. This is just static information. But repeating this information is not ideal. On https://github.com/WordPress/gutenberg/blob/try/global-styles-at-edit-site/packages/edit-site/src/components/editor/utils.js we are adding the mappings to yet another place. This information is going to be needed in multiple, places so we should think about a central way to have it across modules instead of copying each time it is needed.
The way I found to share it between server and client was a setting. I'm not sure what other options we have to centralize this information maybe a JSON file that is read on the server and included on the client too?
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 personally prefer duplicating in these situations. I think we often not think about the use-case of Gutenberg outside the regular post editor context and this kind of setting is just making things harder to reuse the block 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.
I guess if we go the duplication path we should restrict the amount of duplication e.g: one time on the server and another time on the client.
In that case, to share between WordPress modules, I guess the mapping can an experiential constant in wordpress/block-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.
In that case, to share between WordPress modules, I guess the mapping can an experiential constant in wordpress/block-editor?
This works for me, though I don't really understand the need for the mapping in the client as each hook knows what style it produces. Do we really need to have a global variable to hold of that on the client?
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, each hook knows the style it produces but on global styles, we may also have the need to produce and compile styles on the client to have live style previews outside the context of the hook. So essentially these mappings should the shared between the style hook and global styles mechanism.
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 @youknowriad, I updated this PR, now we expose a constant with the mapping. Every module is able to access this data without the need to repeat it.
f4fbb8c
to
175224b
Compare
175224b
to
42953bc
Compare
This PR centralizes global styles mapping information in a single place instead of repeating the mappings.
Fixes: #25051
How has this been tested?
I changed styles on blocks e.g: font size, color, etc And I verified the changes were reflected and serialized on the editor.