Add duotone to image block #26751

ajlende · 2020-11-05T22:45:29Z

Description

This is a subset of #26361 focusing just on the image block since it doesn't have any additional dependencies.

See #26752 for adding duotone as a "block supports" feature.

Add duotone to theme.json
Add duotone filter component
Add disableInserter and disableAlpha props to CustomGradientBar
Add duotone toolbar
Add duotone to image block

The effect is applied as an SVG filter which is supported all the way back to IE 10. Since it's working with SVG, a server-rendered component was required. The SVG is hidden with inline styles, and a stylesheet is written to apply the filter to a specific component using a unique (to the filter) classname.

How has this been tested?

Create image block
Add custom duotone filter
Add preset duotone filter
Test keyboard accessibility

Screenshots

image-block-duotone.mp4

Old Screenshots

Types of changes

New feature

Checklist:

My code is tested.
My code follows the WordPress code style.
My code follows the accessibility standards.
My code has proper inline documentation.
I've included developer documentation if appropriate.
I've updated all React Native files affected by any refactorings/renamings in this PR.

mkaz

This tested well and I didn't come across any issues during testing, it all seems to make sense and no issues with any of the additional components in various testing scenarios.

I made a few suggestions for the PHP written to screen needs some extra escaping, see Data Sanitization documentation for reference.

lib/duotone-filter.php

mkaz

Updates for escaping attributes looks good. 👍

I did further testing and didn't turn anything up. It is looking good to me.

@youknowriad do you want to give it a once over?

youknowriad · 2021-02-26T07:04:53Z

lib/duotone-filter.php

+ * @param  array  $values   R, G, and B values to filter with.
+ * @return string           Duotone stylesheet and SVG.
+ */
+function gutenberg_render_duotone_filter( $selector, $id, $values ) {


Why is this thing not absorbed in the block support?

It's needed later in the image block render and in any block that wants to implement duotone without block supports

I was thinking that ideally, all blocks should rely on the block support and don't implement this adhoc. Is this possible?

Yes, if we go with what you said in #26751 (comment), we can make that happen. 👍

If this is getting moved exclusively to block supports, you can start reviewing the blocks supports parts unique to https://github.com/WordPress/gutenberg/pull/26752/files#diff-fd626ca219f3d805667d480eabb2d890a5a8d2e015b37f03cf8980edf1ae3d58 and WordPress/wordpress-develop#984

youknowriad · 2021-02-26T07:09:36Z

packages/block-library/src/image/edit.js

@@ -275,6 +276,7 @@ export function ImageEdit( {
 	);

 	const classes = classnames( className, {
+		[ duotone?.id ]: duotone,


In #29335 I need a similar unique id to apply inline style for the current block, Do you think we can build a generic thing for that and maybe extract it to its own PR first. It could be a support flag on its own or some utils.

The IDs here don't really need to be completely unique—they just need to be unique to the color combination.

I like the readability of the generated styles this way rather than a pseudo-random unique ID. When I demoed the early prototype at Automattic, I was using the block ID as a unique ID for the stylesheet, but I received feedback that it would be nice if the duplicate duotones could be combined into a single SVG and stylesheet for the page.

If we were doing something generic for outside usage, I think it might be better if we could just have some sort of hashing function rather than generating UUIDs so that we might still be able to do that deduplication eventually. Although, I still haven't thought too deeply about how that deduplication would work.

I personally think unique ids are better because conceptually this is an inline style specific to the current block.
The SVG is not though and yes, it would be good if we can dedup the SVGs (they are duplicated even now) but I don't think it makes sense to base the classname added to the block on the value because we'll be adding a classname per inline style while it's better to have a unique classname for the block and just apply on the inline styles to the same classname.

conceptually this is an inline style specific to the current block

This is very styling-centric, so conceptually for me, this is the same as using something like hue-rotate in CSS. If I was writing this HTML and CSS from scratch, I'd probably have a classname to apply the style, especially if I only have a couple different filters that I would want to apply to multiple images.

It also reminds me that it's possible to inline the style within the CSS. If it were implemented that way, would you think differently about it?

It also reminds me that it's possible to inline the style within the CSS. If it were implemented that way, would you think differently about it?

Potentially yes, but only for presets, for custom duotones, It seems that it's still better to me to have a unique class for the block. Basically, I see it as the same thing as an "inline style" applied to the element style="" since it's a custom value, the only difference is that we can't really use that because we need to target "sub items".

Thinking more here, I can see the point though about considering these as actually "presets" but they are just presets generated dynamically in a single page/post. The problem with that is that right now, they are being generated in the block itself so they are block specific where they shouldn't be if we follow that principle.

So it's one of two things for me:

Embrace the fact that these are inline styles and use a unique id.

Consider these as "dynamic presets" and in that case don't inline the style but instead have a way to "register dynamic custom presets" which means we can dedupe as well.

I still lean towards 1 because it's how we approached things so far for block customizations, and I don't want us to introduce an API/behavior without thinking holistically about it and its impact.

I see it as the same thing as an "inline style" applied to the element

I think I follow your reasoning now. It's more like an inline style because if you change it for one block, it's not going to affect the rest of the blocks that use the same color combination. A thing like "dynamic presets" would probably have to allow for updating everywhere when you update the preset, for example. Centralizing those things as "dynamic presets" would probably make deduplication easier too.

Do you think we can build a generic thing for that and maybe extract it to its own PR first

I don't know if it really needs it. Is it good enough to just use the block id or uuid() like you have in #29335 instead? That's already pretty simple.

yes, I think we should start with just a uuid at first, (left some related comments on the block supports PR)

youknowriad · 2021-02-26T07:10:51Z

packages/block-library/src/image/image.js

+					id={ duotone.id }
+					values={ duotone.values }
+				/>
+			) }
 			{ ( ! RichText.isEmpty( caption ) || isSelected ) && (
 				<RichText
 					ref={ captionRef }


Is there a way to absorb the changes made here in the support flag, meaning if we want to add support to other blocks, there's no changes that should be made in the block itself?

The image block was added separately so the positioning of the toolbar could be controlled. @jasmussen and @mtias said they wanted the duotone toolbar positioned alongside the crop controls or as part of the crop controls which couldn't be done with block supports (See #26361 (comment) and the following few comments)

as part of the crop controls which couldn't be done with block supports

This is not possible now sure but after #29247 it will be possible to use the "block" segment for that. I'm fine if it's added to the end personally until that other PR lands.

youknowriad · 2021-02-26T07:12:24Z

packages/components/src/index.js

@@ -29,6 +29,7 @@ export { default as CardFooter } from './card/footer';
 export { default as CardHeader } from './card/header';
 export { default as CardMedia } from './card/media';
 export { default as CheckboxControl } from './checkbox-control';
+export { default as CircularOptionPicker } from './circular-option-picker';


Why is this (and the bar) exported here? Maybe instead oof exporting this low level component, we could instead add a generic duotone picker UI component here?

The only reason I have it exported is because it seemed like it could be useful for others, and it was more convenient to export that than create a new DuotonePalette like is done for ColorPalette. So I'm guessing you'd prefer exporting a new DuotonePalette instead?

Yeah, I prefer exporting high level components more than lower level ones as it matches what we did before.

I started refactoring it, but I'm having second guesses if that's the right way to go about this.

Creating a new DuotonePalette component in @wordpress/components would be splitting the duotone logic between @wordpress/components and @wordpress/block-editor. getGradientFromCSSColors and getValuesFromColors would need to be used in both packages. Otherwise, if I go ahead and abstract out those parts to keep the duotone functions in @wordpress/block-editor, I'm basically just left with a light wrapper around CircularOptionPicker.

ellatrix · 2021-03-01T08:57:43Z

What's the reasoning for doing this with SVG? Is it not possible to do with CSS filters?

jasmussen · 2021-03-01T09:02:00Z

What's the reasoning for doing this with SVG? Is it not possible to do with CSS filters?

It's not possible. It's possible to get close, but due to the way it's done, colors will always be inaccurate, darker and more muted. Whereas with SVG filters, the actual colors that make up the grayscale of an image can be change directly, allowing for perfectly accurate color choices. You can even make total inversions.

ajlende · 2021-03-24T09:18:54Z

Image block support moved to #26752. See #26751 (comment)

ajlende mentioned this pull request Nov 5, 2020

Add duotone block supports #26752

Merged

6 tasks

ajlende self-assigned this Nov 5, 2020

ajlende added [Block] Image Affects the Image Block [Type] Feature New feature to highlight in changelogs. [Feature] Media Anything that impacts the experience of managing media labels Nov 5, 2020

ajlende mentioned this pull request Nov 5, 2020

Duotone Tracking & History #26361

Closed

ajlende force-pushed the add/image-block-duotone branch from 437ed0d to fc5346f Compare February 11, 2021 03:09

ajlende marked this pull request as ready for review February 11, 2021 21:41

ajlende requested review from ajitbohra, chrisvanpatten, ellatrix, mkaz, oandregal, spacedmonkey and TimothyBJacobs as code owners February 11, 2021 21:41

mkaz requested changes Feb 12, 2021

View reviewed changes

lib/duotone-filter.php Outdated Show resolved Hide resolved

lib/duotone-filter.php Outdated Show resolved Hide resolved

lib/duotone-filter.php Outdated Show resolved Hide resolved

ajlende force-pushed the add/image-block-duotone branch 4 times, most recently from 8fbf926 to a4b76d9 Compare February 15, 2021 22:29

ajlende requested a review from mtias February 16, 2021 18:31

ajlende added 2 commits February 24, 2021 10:09

Add duotone filter

e1b5b58

Add swatch icon

eaf27e6

ajlende force-pushed the add/image-block-duotone branch from a4b76d9 to e77f4a7 Compare February 24, 2021 22:09

mkaz approved these changes Feb 25, 2021

View reviewed changes

ajlende added 3 commits February 25, 2021 20:49

Add duotone to theme.json

aa66ab4

Add duotone toolbar

802581e

Add image block duotone

912b7d0

ajlende force-pushed the add/image-block-duotone branch from e77f4a7 to 912b7d0 Compare February 26, 2021 06:54

youknowriad reviewed Feb 26, 2021

View reviewed changes

Base automatically changed from master to trunk March 1, 2021 15:44

ajlende closed this Mar 24, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add duotone to image block #26751

Add duotone to image block #26751

ajlende commented Nov 5, 2020 •

edited

Loading

mkaz left a comment

mkaz left a comment

youknowriad Feb 26, 2021

ajlende Feb 26, 2021 •

edited

Loading

youknowriad Feb 26, 2021

ajlende Feb 26, 2021

ajlende Mar 1, 2021

youknowriad Feb 26, 2021 •

edited

Loading

ajlende Feb 26, 2021 •

edited

Loading

youknowriad Feb 26, 2021

ajlende Feb 26, 2021

youknowriad Feb 26, 2021

youknowriad Feb 26, 2021

ajlende Mar 1, 2021

youknowriad Mar 1, 2021

youknowriad Feb 26, 2021

ajlende Feb 26, 2021

youknowriad Feb 26, 2021

youknowriad Feb 26, 2021

ajlende Feb 26, 2021

youknowriad Feb 26, 2021

ajlende Mar 1, 2021

ellatrix commented Mar 1, 2021

jasmussen commented Mar 1, 2021

ajlende commented Mar 24, 2021

Add duotone to image block #26751

Add duotone to image block #26751

Conversation

ajlende commented Nov 5, 2020 • edited Loading

Description

How has this been tested?

Screenshots

Types of changes

Checklist:

mkaz left a comment

Choose a reason for hiding this comment

mkaz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ajlende Feb 26, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

youknowriad Feb 26, 2021 • edited Loading

Choose a reason for hiding this comment

ajlende Feb 26, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ellatrix commented Mar 1, 2021

jasmussen commented Mar 1, 2021

ajlende commented Mar 24, 2021

ajlende commented Nov 5, 2020 •

edited

Loading

ajlende Feb 26, 2021 •

edited

Loading

youknowriad Feb 26, 2021 •

edited

Loading

ajlende Feb 26, 2021 •

edited

Loading