Skip to content
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

Move Duotone "presets" blocks support data to dedicated duotone attribute. #48371

Closed
getdave opened this issue Feb 23, 2023 · 16 comments · Fixed by #48426
Closed

Move Duotone "presets" blocks support data to dedicated duotone attribute. #48371

getdave opened this issue Feb 23, 2023 · 16 comments · Fixed by #48426
Labels
[Feature] Colors Color management Global Styles Anything related to the broader Global Styles efforts, including Styles Engine and theme.json [Type] Enhancement A suggestion for improvement.

Comments

@getdave
Copy link
Contributor

getdave commented Feb 23, 2023

As per discussion below let's store any duotone presets (not custom colors which will remain "as is" in the styles.colors.duotone) into a new, dedicated duotone attribute on the block.


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.

Originally posted by @ajlende in #48318 (comment)

@tomdevisser
Copy link
Member

Could you summarize why duotone should go in filters? Feels to me like color is the most logical place. And if not, shouldn't filters at least be part of color? So style.color.filters?

@getdave
Copy link
Contributor Author

getdave commented Feb 23, 2023

@tomdevisser To quote @ajlende here

Conceptually, I think the style attribute was supposed to represent inline styles that were applied to a block. We can't just set

...
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.

@ajlende
Copy link
Contributor

ajlende commented Feb 23, 2023

I'm actually not suggesting adding duotone under filters. Here are some examples of image block attributes for what I'm talking about.

Custom Duotone

Custom duotone filters continue to be stored in style.color.duotone. This example includes a custom border color on the image as well.

{
	"id": 67,
	"sizeSlug": "large",
	"linkDestination": "none",
	"style": {
		"border": { "radius": "12px", "color": "#111111", "width": "8px" },
		"color": { "duotone": ["#000097", "#ff4747"] }
	}
}

Preset Duotone

The preset support added by #48318 would move to duotone (or duotoneFilter if that makes more sense to folks). And this also shows how color presets are currently stored as a top level attribute for border as a comparison.

{
	"id": 67,
	"sizeSlug": "large",
	"linkDestination": "none",
	"style": {
		"border": { "radius": "12px", "width": "8px" }
	},
	"borderColor": "primary",
	"duotone": "blue-red" // This is how duotone needs to be updated for presets.
}

@getdave getdave changed the title Move Duotone block supports to filter key Move Duotone "presets" blocks support data to dedicated duotone attribute. Feb 24, 2023
@getdave
Copy link
Contributor Author

getdave commented Feb 24, 2023

Sorry for any confusion here. I raised this Issue late in my day and got all my terminology mixed up. Thanks to @ajlende for correcting things. I've updated the Issue title and description.

@youknowriad
Copy link
Contributor

Can you explain the reasoning for this change?

Actually as part of the proposal here #37064 and the iterations we're doing to UI components like here, we want to move in the opposite direction for "colors", "typography"...

We want to ideally unify the "style" attribute between global styles and block attribute meaning all "styles" of a given block would be stored in a unified format in the "style" attribute.

@youknowriad
Copy link
Contributor

In other words, what I suggest here is not to create a separate attribute but instead store the value of style.color.duotone as var:preset|duotone|blue-red when we want to refer to a preset. (This format is something that we already use in several places.

@getdave
Copy link
Contributor Author

getdave commented Feb 24, 2023

So #48318 introduced the concept of storing the raw string for the preset in the style.color.duotone block attribute.

So when you select a preset you get the following stored blue-yellow (or whatever).

If you select a custom color Duotone then an array of colors gets stored as it was previously before we supported storing presets.

The change being proposed isn't introduced presets but just where we store those. This is as per #48381.

If that's going to take us off track then we can easily stop and pivot towards something else.

instead store the value of style.color.duotone as var:preset|duotone|blue-red

We could do this but then the block is tightly coupled to the format of the preset. My thought was that it was better to store the preset string only and hide the implementation details from the block. If there's prior art doing it the other way then I can easily update that, but I would like to understand why it's preferable. Much appreciated.

@youknowriad
Copy link
Contributor

var:preset|duotone|blue-red that format is already part of the public API of global styles and I believe some blocks (though to be verified). There are some discussions about the reasoning in the discussion linked above but the idea is that we should have the same "style" format in all Gutenberg. Whether that's global styles or block level. This has several advantages:

  • Makes reusing UI simpler, no need to adapt the format depending on where we are and we can use the same component (see the Typography component above where I was forced to do format mapping due to the difference in formats we have for typography presets)
  • Makes things like: "pusing local changes to global changes" easier as well: again no need to change format or anything, same format is used everywhere.

@youknowriad
Copy link
Contributor

Now you say you already store blue-red in styles.colors.duotone. The question I'd ask here is how do you define that a given block uses a specific duotone preset but in theme.json? Ideally the way it's done in theme.json and the way it's done in block attributes should be similar.

@scruffian
Copy link
Contributor

Here's an attempt to address this: #48426

@youknowriad
Copy link
Contributor

how do you define that a given block uses a specific duotone preset but in theme.json?

Still curious about this, anyone knows what we currently support in theme.json for duotone?

@scruffian
Copy link
Contributor

Still curious about this, anyone knows what we currently support in theme.json for duotone?

You use the CSS variable:

"duotone": "var(--wp--preset--duotone--default-filter)"

@youknowriad
Copy link
Contributor

Interesting, @oandregal @jorgefilipecosta might know better here but are we normalizing everything with the "var" format or am I wrong?

I mean if the decision is to actually use the CSS variables in theme.json, it could make sense to use them in block "style" attribute too. I think we do support both formats for other properties (colors...) but we should probably have a "norm" or recommendation for everyone?

@ajlende
Copy link
Contributor

ajlende commented Feb 24, 2023

I think we do support both formats for other properties (colors...) but we should probably have a "norm" or recommendation for everyone?

The problem with the var(--wp--preset--feature--slug) syntax in block attributes is that the dashes get encoded as \u002d which isn't very readable (ex. var(\u002d\u002dwp\u002d\u002dpreset\u002d\u002dfeature\u002d\u002dslug)). Whereas the colons and vertical line characters get encoded as they are. I assume that's why we use the var:preset|feature|slug format in attributes and internally?

However, when writing a theme.json file by hand it seems better to not obfuscate the fact that you're using a CSS custom property, so we've always recommended using the CSS custom property instead of the weird internal syntax.

@kathrynwp kathrynwp added [Type] Enhancement A suggestion for improvement. [Feature] Colors Color management Global Styles Anything related to the broader Global Styles efforts, including Styles Engine and theme.json labels Feb 28, 2023
@ajlende
Copy link
Contributor

ajlende commented Feb 28, 2023

I think it would be nice if the style attribute only stored valid CSS (or the var: encoding shorthand for CSS custom properties).

I think everything except for duotone is already doing that? Gradients, for example are saved as a CSS gradient string which is then parsed for editing.

One possible way we could make it work for duotone is by encoding the list of colors in the URL. Something like url(#wp-custom-duotone-ffffff-000000), maybe? It might be complicated for colors stored in other formats like hsl since we'd be losing information by converting to hex values.

@youknowriad
Copy link
Contributor

I think it would be nice if the style attribute only stored valid CSS

I don't understand the reasoning behind this? Can you explain further?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Colors Color management Global Styles Anything related to the broader Global Styles efforts, including Styles Engine and theme.json [Type] Enhancement A suggestion for improvement.
Projects
None yet
6 participants