Performance: move style overrides to context #60382
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
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. |
|
Size Change: -295 B (0%) Total Size: 1.73 MB
ℹ️ View Unchanged
|
ellatrix
force-pushed
the
try/style-overrides-context
branch
from
85cc22a to
a999bd2
Compare
ellatrix
requested review from
draganescu,
talldan,
adamziel and
kevin940726
as code owners
ellatrix
force-pushed
the
try/style-overrides-context
branch
from
35ab7f3 to
2ed74db
Compare
ellatrix
commented
Apr 3, 2024
| scope=".editor-styles-wrapper" | ||
| > | ||
| { children } | ||
| </EditorStyles> |
There was a problem hiding this comment.
I guess this is sort of a breaking change, although is experimental.
Maybe there are other ways to achieve the same without the React context.
There was a problem hiding this comment.
In what way is it a breaking change?
There was a problem hiding this comment.
Because the component now accepts children, and blocks must be rendered as children. But this component is experimental so maybe it's ok. Worst case some styles are not applied.
draganescu
reviewed
Apr 3, 2024
| scope=".editor-styles-wrapper" | ||
| > | ||
| { children } | ||
| </EditorStyles> |
There was a problem hiding this comment.
In what way is it a breaking change?
| import { store as blockEditorStore } from '../../store'; | ||
| import { unlock } from '../../lock-unlock'; | ||
|
|
||
| export const updateStyleContext = createContext(); |
There was a problem hiding this comment.
Suggested change
| export const updateStyleContext = createContext(); | |
| export const StyleOverridesContext = createContext(); |
There was a problem hiding this comment.
Context is not a component though
There was a problem hiding this comment.
That is true but it seems using this style is more common. I am ok with styleOverridesContexttoo :)
|
I was looking at style overrides today as well and it seems to me that the main issue is not context vs store. In my tests, to render a single 2024 template, the block editor is setting style overrides 20 times approx. so that's 20 re-renders (even if they're fast). It seems we have "delaying in place" to try to do it in one but it doesn't seem to work. |
|
Correction: EditorStyles is re-rendered 5 times for 25 overrides. |
|
Trying something related here #60493 (not sure how effective) |
|
@youknowriad sure but why not avoid that store all together? |
Because "style overrides" is not the only thing that uses the store, so we'll be creating adhoc context providers and we'll be asking ourselves why not use context for the other stuff too. Store is our context IMO. The other reason is the potential impactful change to EditorStyles. If we were 100% certain that this improves performance, we could try to understand the reasons and address them but it's not entirely clear to me at the moment why context is faster than store. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What?
Move style overrides from the block editor store to context. Originally I thought the block editor store would be best because it could eventually act as the public API, but now we have a much better hook
useStyleOverridethat can be used inside blocks or block hooks.Why?
These style overrides are causing too many dispatches to the block editor store during load. We should avoid polluting the block editor store with unrelated state. Every change causes selectors to be re-evaluated.
1st run -2.5% Replace template modal (Site Editor)
How?
Testing Instructions
Testing Instructions for Keyboard
Screenshots or screencast