Duotone: Use CSS variables instead of slugs in block attributes#48426
Merged
Conversation
|
Size Change: +410 B (0%) Total Size: 1.33 MB
ℹ️ View Unchanged
|
getdave
reviewed
Feb 24, 2023
getdave
left a comment
getdave
left a comment
Contributor
There was a problem hiding this comment.
Seems like a good change if all others blocks conform to storing such presets in the same format.
PS: I fixed the unit tests.
|
Flaky tests detected in 3e2a6cb. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/4264866292
|
added 4 commits
February 28, 2023 10:45
This function is used externally, so we don't want to change the definition of the function
Contributor
|
Testing this out and it's not applying duotone presets to featured images from custom styles applied to themes. Repro:
|
Contributor
|
Debugging notes. Not sure why or what the fix would be, but here's the difference between this branch and trunk that I see for the featured image using custom style:
So, this branch: vs trunk: |
Contributor
|
@jeryj The issue I think you're describing would be caused by what I mentioned in #48318 (comment) Could you try testing it prior to when #48318 was merged. We're trying to get those two changes in the same release so it won't cause issues. |
Contributor
|
Tested it out prior to that commit, and it is working... ish. It has a filter applied but the filter colors aren't right. It sounds like you've got it though! |
ajlende
approved these changes
Feb 28, 2023
ajlende
left a comment
ajlende
left a comment
Contributor
There was a problem hiding this comment.
@jeryj I'm not seeing the issues you are.
Prior to #48318 I see this get saved in the block attributes: "style":{"color":{"duotone":["#000","#C9E4ED"]}. Then I switch to this branch and the old style attribute is still saved and properly rendered as if it was a custom duotone.
Re-selecting the filter switches to the new style attribute "style":{"color":{"duotone":"var:preset|duotone|blue-filter"} which should work when switching themes if the new theme has the same preset.
I'm going to merge this for now so it can be sure to make it into this release and I can make a follow-up if there is something I'm missing.
This was referenced Jul 5, 2023
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
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?
If we save Duotone preset slugs in the style attribute, we should use the standard
var:preset|duotone|*syntax for consistency with other styles.Why?
Fixes an issue introduced in #48318
Resolves #48371
How?
Just prepend the slug with
var:preset|duotoneand then remove it again.Testing Instructions
Testing Instructions for Keyboard
Check that the image has a
filterCSS applied to it and there is an SVG that has a filter with the same id as as the one referenced in the CSS for the image.