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

Duotone: Add a type option #35359

Closed
wants to merge 2 commits into from
Closed

Duotone: Add a type option #35359

wants to merge 2 commits into from

Conversation

scruffian
Copy link
Contributor

@scruffian scruffian commented Oct 5, 2021

Description

This is an alternative to #33673. It allows users to change the type of the duotone filter when defining filters in the theme.json.

There are other valid filters: https://developer.mozilla.org/en-US/docs/Web/SVG/Element/feComponentTransfer
However these require a different format for the SVG. I wonder if it would be better to create an abstraction here, so that we specify "type" as posterize and then let gutenberg_render_duotone_filter_preset determine how to handle it.

How has this been tested

  • In Skatepark, add a type key to the duotone definition, of discrete, and add an image to the post:
                {
                    "colors": [ "#000", "#B9FB9C" ],
                    "slug": "default-filter",
                    "name": "Default filter",
		    "type": "discrete"
                },
                ....

Screenshots

localhost_4759__p=435 (2)

Types of changes

New feature (non-breaking change which adds functionality)

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • I've tested my changes with keyboard and screen readers.
  • 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 (please manually search all *.native.js files for terms that need renaming or removal).

@jasmussen
Copy link
Contributor

Thanks for your patience. Haven't tested yet, but just from looking at the diff this looks like a minimal change. Does this embrace the idea that themes can add entirely custom filters to the menu? I think @ajlende touched around that while refactoring duotone recently.

@scruffian
Copy link
Contributor Author

Does this embrace the idea that themes can add entirely custom filters to the menu?

It's going in that direction. It might be useful to explore what other kinds of filters we want to add, so that we can work out a good API for it now...

@jasmussen
Copy link
Contributor

Yep, I think it's a potent direction, and just overall I'd love to expand the range of filters. The only part that I think is important to work out a plan for (if not a UI, and it might not need to block the PR), is how to categorize and surface the filters in the UI, ensuring a good swatch preview, etc. I've been exploring some more general filter mockups, but they aren't really compelling enough to share quite yet. But if we were to explore additional filters from a theme.json perspective, what might that look like?

@MaggieCabrera
Copy link
Contributor

So you said the way to test this one is to add this under settings/color:

"duotone": [
                {
                    "colors": [ "#000", "#B9FB9C" ],
                    "slug": "default-filter",
                    "name": "Default filter",
					"type": "discrete"
                },

Does that mean we are defaulting to duotone when type is not explicitly defined?

@scruffian
Copy link
Contributor Author

Does that mean we are defaulting to duotone when type is not explicitly defined?

When no type is declared it defaults to table

Copy link
Contributor

@carolinan carolinan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The type is applied correctly in my test, and there are no issues if the type is not defined in theme.json.

@carolinan
Copy link
Contributor

Also please update the theme.json documentation to include the type.

@jasmussen
Copy link
Contributor

This seems okay because of the potential. But before landing this, I'd love a sanity check from @ajlende and/or @oandregal. Not because we might not want this, but just to make sure the UI and/or theme.json structure is prepared for it.

@scruffian
Copy link
Contributor Author

Some other examples of filters we could play with:

Linear:
Screenshot 2021-10-06 at 11 18 25

Gamma:
Screenshot 2021-10-06 at 11 17 23

Discrete but without the colorMatrix:
Screenshot 2021-10-06 at 11 23 35

Table, without the color matrix:
Screenshot 2021-10-06 at 11 25 58

I'm not sure how useful any of these are :)

@carolinan
Copy link
Contributor

carolinan commented Oct 6, 2021

I like the gamma :)

@ajlende
Copy link
Contributor

ajlende commented Oct 6, 2021

I don't think I could make the call for if this particular feature should be supported in the gutenberg plugin. @mtias suggested that he didn't think it should be, but maybe that's just for the UI side of things.

Right now, it's already possible to add the effect via a plugin. While the code is a bit out-of-date, you could follow the old duotone experiment as a reference. However, we'd need to update the gutenberg plugin to support preprocessors if we wanted to have theme.json support.

I previously did a conceptual exploration on how we might be able to add new filters with theme.json because there are so many options for filters, and many of them probably wouldn't fit in the gutenberg plugin. The relevant portion of my notes for this is custom presets UI and preprocessors.

Rather than adding in options to gutenberg_render_duotone_filter_preset, my idea was to allow creation of an entirely new function, a8c_render_custom_image_filter_preset, and add support for it via a plugin like how duotone started out.

In my notes, I imagined a fully restructured theme.json to support it, but most of that restructure was to allow blocks to have multiple selectors. We'd still need to add some additional properties to support custom filters without the full restructure. Maybe it could look something like this.

{
	// New top-level property like `settings`, but for plugins and
	// only focusing on generating presets.
	"pluginPresets": {
		"a8cCustomImageFilter": {
			// We need to somehow tell the theme.json CSS generator
			// how to convert from our preset object into the
			// CSS filter property `url()` string.
			"value_func": "a8c_render_custom_image_filter_preset",
			"values": [
				{
					"name": "Duotone posterize",
					"slug": "duotone-posterize",
					"type": "discrete",
					"colors": [ "#000", "#B9FB9C" ]
				},
				{
					"name": "Linear",
					"slug": "linear",
					"type": "linear"
				},
				{
					"name": "Gamma",
					"slug": "gamma",
					"type": "gamma"
				},
				{
					"name": "Discrete",
					"slug": "discrete",
					"type": "discrete"
				},
				{
					"name": "Table",
					"slug": "table",
					"type": "table"
				}
			]
		}
	},
	"styles": {
		"blocks": {
			"core/image": {
				"filter": {
					// We could utilize the duotone filter here because
					// it basically just signifies that the filter
					// should be added to the selector defined in the 
					// `__experimentalDuotone` supports in block.json.
					"duotone": "var(--wp--custom--a8c-custom-image-filter--duotone-posterize)"
				}
			}
		}
	}
}

So, sorry I can't give a "yes" or "no" for this, but hopefully what I've written can add a little more insight to filters as plugins if this ends up not being something for the gutenberg plugin.

@mtias
Copy link
Member

mtias commented Oct 7, 2021

I think there's room for something like this but we need to think through the instrumentation a bit more since it doesn't make sense to tuck everything under duotone.

@scruffian
Copy link
Contributor Author

Closing this old PR.

@scruffian scruffian closed this Mar 18, 2024
@oandregal oandregal deleted the add/duotone-type branch March 19, 2024 13:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants