Fix dynamic references on the site editor #42976
Merged
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
MaggieCabrera
requested review from
getdave,
scruffian,
oandregal,
ajlende and
draganescu
MaggieCabrera
added
the
Global Styles
|
Size Change: +1.84 kB (0%) Total Size: 1.27 MB
ℹ️ View Unchanged
|
getdave
reviewed
Aug 4, 2022
packages/edit-site/src/components/global-styles/use-global-styles-output.js
Outdated
Show resolved
Hide resolved
packages/edit-site/src/components/global-styles/use-global-styles-output.js
Outdated
Show resolved
Hide resolved
packages/edit-site/src/components/global-styles/use-global-styles-output.js
Outdated
Show resolved
Hide resolved
packages/edit-site/src/components/global-styles/use-global-styles-output.js
Outdated
Show resolved
Hide resolved
packages/edit-site/src/components/global-styles/use-global-styles-output.js
Outdated
Show resolved
Hide resolved
|
Whoops! Forgot to leave a comment on the review. This is looking really great 👏 Some nits and suggestions. Main one being tests. |
Co-authored-by: Dave Smith <[email protected]>
|
Looks like the editor is still crashing, I was looking at the wrong place 🤦♀️ I'll keep working on this |
The crash is because of the value being parsed in the UI. I think we need two fixes, and this is one of them... |
Exactly, I'm not sure where the fix should go for the GS! |
scruffian
reviewed
Aug 4, 2022
packages/edit-site/src/components/global-styles/use-global-styles-output.js
Show resolved
Hide resolved
|
I can confirm this fixes part of the issue - links are now correctly styled in the Site Editor. |
|
This now fixes the editor crashing on Global Styles. I tested changing the colors, and that works too. @ndiego it looks like this fixes #43033 too, can you confirm? Thanks @scruffian for your help figuring this one out |
|
Thanks @MaggieCabrera, just tested this PR against #43033 and color changes are working as expected in Global Styles. 🙌 |
|
I only wonder what about the other uses of |
|
as for that, I don't think there are at the moment, no |
|
@scruffian @draganescu I refactored this a bit to avoid the confusing function. I'm not sure that we need a specific function just to get the ref right now, but I don't have a strong opinion on the matter. |
|
I have not tested but it looks better as a patch that solves the missing case in the existing flow instead of introducing duplication. My main question is if there is any performance impact from using a larger object (the whole tree) every time. How can that be tested? |
the larger object is never used when it's not needed in the end. It's passed as an argument but only used by the function getting the rev value, the rest of the functions use the settings part of it only. |
|
Hey, tested this a bit and found a case we need to prevent from happening: recursive references. It can be tested by adding the following to the |
This is already covered by this check. Should we do something more than that? |
|
The editor still crashes with that check:
|
I think we should merge this as it fixes the main problem and then follow up with another check for this case. |
scruffian
approved these changes
Aug 11, 2022
I opened #43154 |
Ok great, thank you. |
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?
Fixes #42890
Why?
https://github.com/WordPress/gutenberg/pull/41696/files implemented references on the frontend but they weren't being resolved in the site editor. In particular, they were breaking Global Styles.
How?
I created a function that resolved the references into the actual values being referenced. I'm not sure if this is the best place or best method to do this, but it solves the editor crashing because of the bug and keeps the editor consistent with the frontend. A next step would be to discuss how we want to surface this link to the users like @oandregal mentioned.
Testing Instructions
With this PR checked, follow the instructions from #42890 and see that the site editor doesn't crash when visiting the links section.
Screenshots or screencast
Rainfall uses refs for links too, you can test it there and see it doesn't crash: