-
Notifications
You must be signed in to change notification settings - Fork 4.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add duotone theme.json styles support #34667
Conversation
081c490
to
4c7bc53
Compare
Size Change: -1.47 kB (0%) Total Size: 1.06 MB
ℹ️ View Unchanged
|
/** | ||
* Metadata for style properties that need to use the duotone selector. | ||
* | ||
* Each element is a direct mapping from the CSS property name to the | ||
* path to the value in theme.json & block attributes. | ||
*/ | ||
const DUOTONE_PROPERTIES_METADATA = array( | ||
'filter' => array( 'color', 'duotone' ), | ||
); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@oandregal I see this as a stepping stone to eventually get us to a better way to configure styles that need different selectors. Since these selectors are still experimental (__experimentalSelector
and __experimentalDuotone
), I didn't want to worry too much about the final API yet.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is nice but has the drawback to have to consider the two arrays in other methods (remove_insecure_styles
). I've pushed a tweak and commenting at #34667 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've reverted the change I had done, so we need to make this work with remove_insecure_styles
. There're some tests we can update to consider duotone at WP_Theme_JSON_Gutenberg_Test.test_remove_insecure_properties_removes_unsafe_styles
7a38413
to
82dd4b9
Compare
82dd4b9
to
bbdd7ab
Compare
741c0ee
to
ec86c0b
Compare
b5f777f
to
8c421cc
Compare
// !important is needed because these styles render before global styles, | ||
// and they should be overriding the duotone filters set by global styles. | ||
$filter_style = defined( 'SCRIPT_DEBUG' ) && SCRIPT_DEBUG | ||
? $selector . " {\n\tfilter: " . $filter_property . " !important;\n}\n" | ||
: $selector . '{filter:' . $filter_property . ' !important;}'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it be safe to add specificity by prepending this with .wp-site-blocks
instead of using !important
? Can I count on having that as a container? Is it weird to use body
instead of that class if it's not available?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you can rely on .wp-site-blocks
, but i do wonder if this is a valid use of !important
- since this is a setting on an individual block, I think it should always take precedence over other CSS.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was mention of a similar situation for elements block supports in the #core-editor Slack today.
The linked issue is #33437 and there's a fix by removing !important
in #34689. That issue is caused by the cascade and because sometimes buttons, that we want to style differently, use the <a>
tag.
The filter property doesn't cascade the same way, so we're probably fine here. But the elements block supports goes to show that just because we're setting an individual block, !important
still might not be safe to use.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a specific need for the duotone filters to be enqueued before the global styles?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, if we can make it work without the !important
is nice.
The issue with #33437 was that .wp-element-* a
targeted any a
element in a container (group with paragraph with link + button + social link blocks). So the issue was about how unspecific the selector was. Which makes me think: is it possible that the container with the .wp-duotone-ID
has an image with duotone and another without it? I'm just not familiar with all the blocks this can affect to. I can't think of any off the top of my head.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
More context about where we use !important
: we use it in the preset classes, such as .has-{value}-color
to make sure the user selection is respected despite its low specificity. More context #29533
In the case of the link color, it was probably too much due to the wide reach of the selector and side-effects. We're reverting it in this case if the current PR seems solid.
Hey, this isn't working. Apparently, we create the CSS Custom Property |
The current filters work in a couple of situations: when the user doesn't have Note that there's the |
Check your That being said, I forgot to update the duotone ids to match the CSS variables, so I've taken care of that in df50ef0. |
Oh, brilliant, I did have the older name in my |
Talked to @mtias and he gave the following reasoning for putting it under
Updated in 008c5d5 |
Thank you for this! I'm implementing this for Skatepark and I found a couple minor issues in the editor/inserter: The thumbnail for block patterns that use duotone won't show the filter applied: The site editor shows the image without a filter but the frontend works fine. The post editor when I insert an image does work fine: You can test on Skatepark applying Automattic/themes#4740 |
The site editor must not render the And the thumbnail is going to be tricky because its in an iframe—we're going to need some way for duotone to put the SVG filters into that iframe. I have an idea for how to fix the site editor that I can get to tomorrow. But I'm not sure where to start for the thumbnail, so if anyone has thoughts on a good way to go about dealing with that iframe let me know. |
CC: @ellatrix in case you have any thoughts on the thumbnails. Alex do we need to load into that iframe an additional stylesheet? |
Just so you know, the thumbnails were working with duotone when the filter was directly applied in the markup of the block. It just broke after my PR linked above for Skatepark. |
I've prepared #35255 allows presets to use the newly introduced |
#35318 allows users to store duotone data via |
Description
Alternative to #34073
Fixes #33642
url()
property instead of stylesheet + SVG.duotone
selector for rendering duotone filter styles.value_func
andvalue_args
options fromPRESETS_METADATA
.There are some features mentioned in #34037 that are not included here because they may be able to be implemented as follow-up PRs.
.has-<slug>-duotone
classesTesting Procedure
Configure theme.json
Add
var:preset|duotone|<slug>
orvar(--wp--preset--duotone--<slug>)
to your theme.json understyles.blocks.<block>.color.duotone
.<slug>
is the kebab-case slug for a duotone filter that a theme supports found undersettings.filter.duotone
in theme.json.<block>
is the block to apply the filter to. It can be any block that supports duotone.For example, on TT1 Blocks, you can add the following to the theme.json.
Or for Skatepark you can add the following to the theme.json.
Add blocks with theme duotone styles
Add the supported blocks to the post to see the default filter applied.
Override duotone per block
Select a different duotone filter from the duotone toolbar to see the default filter overridden.
Generated Styles
For the Skatepark example, these are the new global styles that get generated. The duotone filter SVGs get rendered separately in the footer because Safari doesn't like them as data URIs.
Each custom duotone preset override generates a style like this in the
<head>
.Screenshots
Using Skatepark with
blue-filter
as the default filter in the left column overridden byblue-cream-filter
in the right column.Types of changes
New feature