Editor: Add global styles data to editor settings #59929
Conversation
|
Size Change: +834 B (+0.05%) Total Size: 1.74 MB
ℹ️ View Unchanged
|
michalczaplinski
added
[Type] Enhancement
| @@ -237,6 +241,7 @@ function useBlockEditorSettings( settings, postType, postId ) { | |||
| BLOCK_EDITOR_SETTINGS.includes( key ) | |||
| ) | |||
| ), | |||
| __experimentalStyles: styles, | |||
There was a problem hiding this comment.
The settings already contain a styles key (it's in the settings object above)
This styles object contains the "compiled" styles, (the CSS) which is not what we want here. What we want is the styles object that contains the user styles.
Another alternative to adding a new __experimentalStyles key might be to nest both the "compiled" and "uncompiled" styles under settings.styles:
settings.styles.raw- the "uncompiled" styles (what we're adding here)settings.styles.css- the styles currently present undersettings.styles
The naming is of course debatable, I'm only suggesting what the structure might be.
There was a problem hiding this comment.
Yes, I think the main blocker of this PR is the naming. It's also important in the discussion to consider that we also have __experimentalFeatures that corresponds to the "settings" part of global styles and this new key corresponds to the "styles" part.
The other thing to note is that some of the "compiled" styles mention above are a duplication of the "raw" styles but not all of them. Some other styles come from CSS files themes or plugins inject.
There was a problem hiding this comment.
@WordPress/gutenberg-core We need your help coming up with the best API here for the block editor package (generic package to build block editors). How should we pass the "global styles" styles and settings and what about "extra styles" that can be provided as raw CSS to be applied to the editor canvas.
There was a problem hiding this comment.
At 1st sight I like the idea to split styles in two:
styles.source
styles.compiled
I think the concept of raw doesn't apply since we usually take something raw and transform it into a prepared version. In this case tho we compile to CSS some JSON data.
There was a problem hiding this comment.
What we want is the styles object that contains the user styles.
Why can't we then call the key userStyles?
There was a problem hiding this comment.
Maybe rename this new attribute to globalStyles and rename the existing styles attribute to css?
There was a problem hiding this comment.
rename the existing styles attribute to css?
Wouldn't that break back compat?
There was a problem hiding this comment.
+1 for globalStyles
It covers the potential to include theme and user-defined styles as noted above.
There was a problem hiding this comment.
rename the existing styles attribute to css?
Wouldn't that break back compat?
I should have specified "rename in a backwards compatible way" :) so support both but prefer css. Could also look at deprecate()ing styles but it might be too noisy.
There was a problem hiding this comment.
Good discussion!
+1 for renaming/repurposing styles - globalStyles is as good as any name. Will there ever be customStyles or userStyles, or do we assume that global styles will be theme + user merged?
Even if it's noisy for a while to rename/deprecate, I don't think backwards compatibility should be so stultifying a mandate that it should get in the way of progress! Onwards! 🤣
|
Thanks for working on this!! I think this has been tried (and abandoned) before:
So very happy to see it revived! |
|
Another +1 for this. Access to the global styles data is also required for the enhanced block style variations feature: |
|
Thanks for the links, @ramonjd! I didn't get to review the previous PRs before my AFK so will have to get back to it the week after the next. |
No worries, I was just dumping them here for cross-linking purposes. Hopefully it will help future Github archaeologists 😄 |
|
I've been thinking about this, the main blocker is that we're not able to decide on a name for this setting. Can we start by using a private setting then? We already have a |
| const { getEditedEntityRecord, hasFinishedResolution } = | ||
| select( coreStore ); | ||
| const _globalStylesId = | ||
| select( coreStore ).__experimentalGetCurrentGlobalStylesId(); |
There was a problem hiding this comment.
Should we combine those 2 select( coreStore ) into a single one?
| // Make sure to recompute the styles when hasResolved changes. | ||
| const config = useMemo( () => { | ||
| return { | ||
| styles: styles ?? {}, |
There was a problem hiding this comment.
Should this always default to the same empty object rather than defining a new one on every computation?
|
Is there a simple feature or an enhancement we can include as part of this PR that uses |
|
How does this new hook relate to |
|
@noisysocks There's a chance it might be used in #57908, as that requires the uncompiled styles atm. Context in this discussion thread - #57908 (comment). Still investigating whether it's the right fit though and what's left to do. |
This is correct 👍 FWIW I've taken this PR's approach for a quick spin alongside #57908 and it works well so far. If I understand the discussion to date, the main sticking point is just the key this styles data will live under?
This is a good question. Should the global styles data always be available in the editor or only if the editor has that context? If it is the latter, #57908 would require the post editor set up the GlobalStylesContext provider as per the site editor. I suspect the style inheritance work would have the same need too. |
aaronrobertshaw
force-pushed
the
try/styles-inheritance-hok
branch
from
5ed25f3 to
a3ada64
Compare
|
I've rebased this PR to resolve a few conflicts and prepare for merging #61022 back into this one. |
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
|
I've merged into this branch #61022 that addresses most of the feedback received either here on on that PR. It also updates the approach slightly so that |
aaronrobertshaw
changed the title
| return { | ||
| isReady: isBaseStylesReady && isUserStylesReady, | ||
| base: baseStyles ?? DEFAULT_STYLES, | ||
| user: userStyles ?? DEFAULT_STYLES, |
There was a problem hiding this comment.
Is it a good idea to return base, user and merged, or should we just return merged?
There was a problem hiding this comment.
I did wonder about this myself.
It could be argued returning the different origins' data is more future-proof. For example, the style inheritance UI could differ based on at which level the style was modified.
The current approach also reflects the data in the site editor's global styles context providing more consistency. Which could help consolidate some of this down the road.
There was a problem hiding this comment.
I'm not entirely sure here. If one think about the block editor package as a standalone package, it doesn't really know that there are multiple levels of style configurations and probably doesn't matter for blocks where the style is coming from. I'd love to have more details about the potential UIs before committing to this.
There was a problem hiding this comment.
think about the block editor package as a standalone package, it doesn't really know that there are multiple levels of style configurations
That's a good point, thanks @youknowriad 👍
One other option I was pondering was only adding the merged property for now keeping the door open to add the others later if needed.
To your point though maybe I should switch this PR to only return the merged values.
@richtabor can probably confirm the style inheritance UI work only needs the merged data.
There was a problem hiding this comment.
I'd love to have more details about the potential UIs before committing to this.
I don't think that we need much (perhaps any) UI for style inheritance. If we can get the default values available on blocks, and leverage the existing reset/clear UI.
For example, this could be an unedited group block, with its styles derived from global styles, with a font size of XL applied locally:
I'd expect resetting the font size would set it to whatever the current global styles value is (regardless of if its theme or user).
There was a problem hiding this comment.
So it looks like we only have a use case for the merged data so far. I'll get this updated to only add the merged styles to the block editor settings.
There was a problem hiding this comment.
I've updated this PR to return only the merged styles data. Let me know if there is anything else that needs tweaking.
There was a problem hiding this comment.
Rethinking this a bit, I'd like to reduce some code duplication with the site editor's global styles context setup.
This would require the user, base, and merged configs to be exported from a new hook in the editor package. The editor setting however will still only be the merged style data as discussed.
I'll update again when this is ready for another look.
There was a problem hiding this comment.
There's an alternate approach to this PR up in #61556.
TL;DR
The new approach relocates the collection of GlobalStylesContext data to the editor package. This way we have a single location that collects and merges the global styles data. So when block style variations need to be resolved within that merged data, it only happens once and is available in both editors.
After moving the GlobalStylesContext it seemed to make sense that the GlobalStylesProvider was also in the editor package. Happy to move if required.
aaronrobertshaw
force-pushed
the
try/styles-inheritance-hok
branch
from
2ef52a7 to
2e70b96
Compare
|
Closing because #61556 is merged |
Original PR Description
What?
This PR introduces a new
useGlobalStylesData()hook in theeditorpackage that gets the user global styles. Those user styles are then added to the block editor settings.Why?
Second attempt at building a foundation to fix #49278 and #43082.
The first attempt (#55952) is probably not the right approach.
How?
We add the "uncompiled" global styles to the editor settings.
What?
Adds merged global styles data to the block editor settings.
Why?
The global styles data is required in both editors for two main areas of ongoing work:
How?
useGlobalStylesDatahas been added to the editor package but not exported as a public API.Testing
As this PR adds the data to a private key within the block editor settings, it can't easily be inspected via Redux dev tools.
The following snippet will log out the global styles data via a
console.debug()call when there is a block with an element style applied. From there you can easily confirm the correct data. Bonus points if you make tweaks in theme.json and global styles, then reconfirm that all gets merged properly.Test snippet