Global styles: resolve dynamic refs #43249
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
ramonjd
added
the
Global Styles
|
Size Change: -35 B (0%) Total Size: 1.24 MB
ℹ️ View Unchanged
|
|
I'm actually getting a crash in the editor when I go to edit in global styles the text color:
|
Thanks for testing @MaggieCabrera There's also an error when I select the background color control.
It makes me think we need to deal with these value at a higher level, either the point at which we first parse incoming theme settings in the JS or on the server 🤷 What do you reckon? |
The editor did not correctly resolve dynamic refs and the style engine does not yet support resolution. This commit adds a utility function to resolve dynmamic refs and add conditions to prevent recursive refs.
…k with a string value, albeit empty.
ramonjd
force-pushed
the
update/global-styles-to-check-for-dynamic-refs
branch
from
8510bb2 to
7e71b16
Compare
Yeah, I think it makes sense that the frontend just gets values that need to be resolved but that we know are correct beforehand |
|
Closing this. A bit stale and there doesn't appear to be an immediate need. |
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?
A follow up for #43166
Why?
To clean up how we resolve dynamic refs in the editor, remove duplicated code.
Also, because the style engine doesn't yet handle dynamic refs, moving the logic outside the
getCSSRulesloop.marginand probably some other props were being skipped because of this.How?
This commit adds a utility function to resolve dynamic refs and add conditions to prevent recursive refs.
Testing Instructions
I'm using empty theme to test!
Here is some test theme.json
{ "version": 2, "styles": { "color": { "text": { "ref": "styles.color.background" }, "background": { "ref": "styles.color.text" } }, "typography": { "letterSpacing": { "ref": "styles.spacing.blockGap" } }, "spacing": { "margin": { "ref": "styles.spacing.blockGap" }, "blockGap": "11px" } } }What we want to do is make sure the editor styles in
.editor-styles-wrapperare displayed as they are on the frontend.You'll see a PHP warning:
This is expected because of the recursive reference.
Check that the test case in #43166 is still valid. That is, make sure the editor doesn't crash. 😄
Run the tests!
npm run test:unit