-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Block editor: remove __unstableElementContext
and filter EditorStyles
instead
#52888
Conversation
@@ -163,11 +163,9 @@ function getPresetsSvgFilters( blockPresets = {} ) { | |||
.filter( ( origin ) => presetByOrigin[ origin ] ) | |||
.flatMap( ( origin ) => | |||
presetByOrigin[ origin ].map( ( preset ) => | |||
renderToString( |
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.
Notice how we also get rid of this renderToString
call.
Size Change: +1.17 kB (0%) Total Size: 1.53 MB
ℹ️ View Unchanged
|
Flaky tests detected in 715d47f. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/6107870529
|
@ellatrix, the description mentions |
@Mamaduka Fixed the link. Github has changed some of the links to relative hash links, so when I paste links into Github they assume its the same PR... |
I like the idea of keeping every style preset in a single location/setting. This matches behavior on the PHP side. Concerns
|
A proposal to prevent the race conditions: #53516 |
097b351
to
935f69a
Compare
Ok, rewrote the PR to filter the styles instead of going through editor settings. |
__unstableElementContext
and filter EditorStyles
instead
Okay, great. Thanks for confirming @youknowriad! @ellatrix I think from the sounds of it there's nothing to worry about in this PR, then, and we can explore our options for the render behaviour in follow-ups 🙂 |
935f69a
to
0a0949e
Compare
Cool thanks! It sounds good if other ideas are explored. My goal with this PR however is to remove the portals and consolidate around |
@Mamaduka I addressed your feedback :) |
deleteStyleOverride, | ||
] ); | ||
|
||
return null; |
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.
Minor: If it's not a public Api, this DuotoneStyles
component could probably be a useDuotoneStyles
hook since it renders nothing.
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.
Well, that's exactly the whole point of this PR 👍
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.
Actually, I do want to wait a bit with until we refactor duotone more. There's a shouldRender
condition that would be removed. It would be good if we can prevent a bunch of hooks like useSetting from doing anything if it's not needed for that block. I'd rather wait when we look into refactoring this whole API so we don't impact performance.
I didn't test but this is very cool 👍 |
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.
@ellatrix, I see the following error when deleting the block with Duotone filters applied.
Screenshot
Fixed |
e582e07
to
f5341ea
Compare
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.
The duotone changes test well for me with various blocks. I couldn't spot any regressions.
I wonder if this is the same issue that @andrewserong discovered here: #54493 (review). |
I don't think so, at least not if it's related to id collision, because the blocks that aren't getting their styles output all have different ids. |
Just added a comment on the linked issue (#54956 (comment)), this sure is a weird one! I'm wondering if previews might have issues since they have settings passed down to their block editor providers from above, so updates to state within the previews might not be propagating up to the "real" block editor store? |
@ellatrix Any ideas why this method of adding styles to editor settings no longer works for me in the Site Editor but still works in Post Editor (WP 6.3.2/6.4-RC2). As for
it re-transforms every style:
For me this is performing very badly as I can have hundreds of styles. |
What?
TO DO: do think for every filter using
BlockList.__unstableElementContext
, but if it works for Duotone, it will work for the rest too. We can look at these in separate PRs.This PR aims to eliminate
BlockList.__unstableElementContext
, which is currently only necessary to add style elements and SVGs. While working on Allow styles to be changed dynamically through editor settings, I realised that we could use thestyles
block editor setting instead. I also realised that we're actually using this for the Duotone Global Styles Presets, so this make it even more appealing.What we can do is filter the editor styles setting through the
EditorStyles
component through context: pass down a callback to update/clear a given style with id. Not only can this be used to add new styles with new ID, but you can also filter/take over server rendered styles.For now, this is better because we no longer need to render anything in the React tree and it makes it easier to design a new API.
Why?
Currently, we can't refactor
editor.BlockListBlock
because of the need to portal styles and svgs. This PR removes this need.How?
Testing Instructions
Testing Instructions for Keyboard
Screenshots or screencast