Use Duotone presets in block duotone attributes #48318
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
getdave
requested review from
scruffian,
MaggieCabrera,
jeryj,
ajlende and
draganescu
|
Size Change: +130 B (0%) Total Size: 1.33 MB
ℹ️ View Unchanged
|
getdave
force-pushed
the
try/use-duotone-presets-for-blocks
branch
from
dcbc635 to
47f1599
Compare
|
Flaky tests detected in a5c64eb. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/4251787158
|
|
We need to think about how this will intersect with #48291, since these presets may not be available anymore... |
Todo
|
|
I think it would be better to deal with that in this PR, since this PR deals with the rendering of blocks that use specific presets, so we should make sure that we output the necessary SVGs when the block is rendered. |
Can you elaborate on how you think this PR needs to change in order to accommodation what you are doing in #48291? |
Sorry, on reflection I think it makes more sense to keep this PR as is and update the approach in the other PR :) |
jeryj
reviewed
Feb 22, 2023
jeryj
reviewed
Feb 22, 2023
ajlende
reviewed
Feb 22, 2023
| @@ -311,9 +311,6 @@ function gutenberg_get_duotone_filter_id( $preset ) { | |||
| * @return string Duotone CSS filter property url value. | |||
| */ | |||
| function gutenberg_get_duotone_filter_property( $preset ) { | |||
| if ( isset( $preset['colors'] ) && 'unset' === $preset['colors'] ) { | |||
| return 'none'; | |||
There was a problem hiding this comment.
I wish I would have used 'none' as the string instead of 'unset' so that string values could just be treated as CSS like the rest of the block supports. But for maintaining backwards compatibility, I don't think that's really an option anymore.
There was a problem hiding this comment.
Just so you know this return value is preserved but I moved it outside of this function so it doesn't need colors in the "preset".
ajlende
reviewed
Feb 22, 2023
lib/block-supports/duotone.php
Outdated
Comment on lines
445
to
449
| // Possible values for duotone attribute: | ||
| // 1. array('#000000', '#ffffff'). | ||
| // 2. 'preset-slug'. | ||
| // 3. 'unset' (remove filter). | ||
| $duotone_attr = $block['attrs']['style']['color']['duotone']; |
There was a problem hiding this comment.
I do think it's worthwhile in this PR to keep the styles separate from the presets rather than moving it to a follow-up.
Conceptually, I think the style attribute was supposed to represent inline styles that were applied to a block. We can't just set <div class="wp-block-image" style="filter:url('data:image/svg+xml...');">...</div> because kses strips SVGs and data URL filters don't work in Safari anyway. But conceptually, that's what it means.
Whereas, the top-level attributes (textColor, gradient, etc.) represented presets that were usually applied with a class on the block wrapper. Again, we can't just apply a class to the wrapper because filter is a non-inherited CSS property that we want to apply to a subset of the block.
So I think a top-level duotone attribute is more appropriate for saving the presets to.
There was a problem hiding this comment.
I agree it's needed...but that's going to involve:
- deprecations
- additional refactoring to work with the new attribute whilst maintaining the backwards compatibility
...I still think I would be better to make an immediate follow up and avoid bloating this PR.
As soon as I feel I would need to use the word and in a PR description to accurately reflect its purpose it's become too large.
Is there a reason it has to be done in this PR?
There was a problem hiding this comment.
Why do you think it would need deprecations or additional backwards compatibility code?
The way I see it, old blocks with the custom colors saved continue to work like they did before. Nothing here gets touched.
New blocks that select a preset have a new attribute that unsets the style.color.duotone attribute and sets the new duotone attribute.
I think that it should be this PR so that we don't have to deal with backwards compatibility when switching over to storing custom filters in style and presets in duotone.
There was a problem hiding this comment.
Eeeek. Too late. But I'll do a follow up 😶
There was a problem hiding this comment.
So we should end up with
attributes.filters.duotone- stores duotone presets only.attributes.style.color.duotone- stores custom duotone only.
Is that right?
draganescu
reviewed
Feb 23, 2023
|
Feel free to review this one. I'm going to add unit tests for the x2 utility functions. |
|
I found that if I added a custom Duotone, it didn't work correctly in the frontend. This was the Duotone I added:
|
|
I tested this and it applied my custom duotone somehow wrong: My custom duotone
Render of the custom duotone
Original image
|
|
@draganescu @scruffian Thanks for catching custom color breakage. I've reinstated some code I accidentally removed during a refactor. Points to this code being brittle and requiring tests. |
This was referenced Feb 23, 2023
Duotone color arrays can be more than 2 colors, and the code doesn't seem to handle 2 colors differently than 3 or more, so the count() === 2 was unnecessary. Since we've already defined the $is_duotone_colors_array, we can use it later instead of redoing another is_array() check.
Previously the decision about how to handle values were deferred to the utility. Instead the consuming code should decide whether to look for a preset.
getdave
force-pushed
the
try/use-duotone-presets-for-blocks
branch
from
5f049de to
a5c64eb
Compare
|
So I'm not entirely certain but I believe this PR might have degraded the "typing" performance by 5ms (which is like 10%).
I think that's substantial for a single PR. So I'm wondering whether we should try to revert this in a PR to see the impact it has and potentially find the root cause of the regression. |
This was referenced Jul 5, 2023
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?
Uses Duotone presets (where available) when storing duotone for a given block.
Part of #46547
Why?
Currently when setitng Duotone on a given block (e.g.
core/image) the duotone is stored as an array of colors:['#000', '#fff']However this isn't useful if you switch Themes. Be storing slugs of Duotone presets we can take advantage of portability of color swatches across Themes.
How?
This PR checks the selected Duotone swatches against known Duotone preset values and if one is found the slug of the matching Duotone is stored in the block attributes.
If no preset is found then an array of custom colors is stored as per
trunk.Testing Instructions
Testing Instructions for Keyboard
Screenshots or screencast