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

Add duotone block supports #26752

Merged
merged 8 commits into from
Apr 22, 2021
Merged

Conversation

ajlende
Copy link
Contributor

@ajlende ajlende commented Nov 5, 2020

Description

This is a subset of #26361 focusing just on adding duotone as a "block supports" feature. It required some PHP changes to server-render the SVG and stylesheet, so those changes can be found at WordPress/wordpress-develop#984.

See #26751 for adding duotone to the image block which doesn't require the core changes.

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.

One concern I have for applying this to videos is that the duotone effect is also applied to the video controls. This may be a problem if the duotone effect does not have high enough contrast.

EDIT: After some feedback, support was removed for the blocks that have video controls until separate controls can be added without the duotone filter applied. There are also no longer core changes required to make this work.

How has this been tested?

  1. Create video, media & text, or cover block
  2. Add custom duotone filter
  3. Add preset duotone filter
  4. Test keyboard accessibility

Screenshots

cover-block-duotone.mp4
Old Screenshots

media-text--block-duotone.mp4
video-block-duotone.mp4

cover-duotone-2020-11-05

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.

@ajlende ajlende mentioned this pull request Nov 5, 2020
6 tasks
@ajlende ajlende self-assigned this Nov 5, 2020
@ajlende ajlende added [Block] Media & Text Affects the Media & Text Block [Feature] Media Anything that impacts the experience of managing media [Type] Feature New feature to highlight in changelogs. [Type] New API New API to be used by plugin developers or package users. and removed [Type] New API New API to be used by plugin developers or package users. labels Nov 5, 2020
@ajlende ajlende changed the title Add/block supports duotone Add duotone block supports Nov 5, 2020
@ajlende ajlende force-pushed the add/block-supports-duotone branch 2 times, most recently from 6e89845 to 5260dc3 Compare February 11, 2021 19:40
@ajlende ajlende marked this pull request as ready for review February 12, 2021 00:07
@ajlende ajlende force-pushed the add/block-supports-duotone branch 4 times, most recently from ecd5a45 to 32a37ae Compare February 15, 2021 22:30
@ajlende ajlende force-pushed the add/block-supports-duotone branch 2 times, most recently from 6860884 to 533b01a Compare February 26, 2021 06:56
@ajlende ajlende force-pushed the add/block-supports-duotone branch from 50dbbb0 to 9a0900b Compare April 13, 2021 00:05
@ajlende ajlende force-pushed the add/block-supports-duotone branch 2 times, most recently from fc9ff0b to 7a79c4b Compare April 20, 2021 08:31
@ajlende ajlende force-pushed the add/block-supports-duotone branch from 7a79c4b to c341bd2 Compare April 20, 2021 08:42
@ajlende
Copy link
Contributor Author

ajlende commented Apr 20, 2021

@youknowriad @nosolosw I've gone through and updated things based on your comments. It should be ready for another review pass. Thank you!

Copy link
Contributor

@youknowriad youknowriad left a comment

Choose a reason for hiding this comment

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

This is a very well driven PR. Thanks a lot for your work and iterations. I really appreciate the documentation effort as well for the block support.

I left some small comments and I think it's lacking some testing (php unit tests for the block support or e2e tests) but most of these can be good follow-ups.

* @param string $color_str CSS color string.
* @return array RGB object.
*/
function gutenberg_tinycolor_string_to_rgb( $color_str ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if can add a couple of unit tests for these functions. Or maybe if an existing php library could be used for that.

Copy link
Member

@aristath aristath Apr 21, 2021

Choose a reason for hiding this comment

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

Well... I'm currently maintaining https://aristath.github.io/ariColor/ so I could definitely improve it, give up ownership, and even rewrite it if we want to use a library. It's been battle-tested in hundreds of themes already since it was part of a customizer framework 😉

Copy link
Contributor

Choose a reason for hiding this comment

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

@aristath Does it mean this is already in Core? in which case we could just directly use It here?

Copy link
Member

Choose a reason for hiding this comment

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

No, not in core... It was in a customizer framework that themes were (are?) embedding to get extra controls & functionality.

Copy link
Contributor Author

@ajlende ajlende Apr 21, 2021

Choose a reason for hiding this comment

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

Tinycolor also has some quirks with rounding numbers with colors and doesn't follow the whole CSS spec. I was going for consistency over everything else for this which is why things are the way they are.

I've been slowly working on https://github.com/ajlende/color-fns/ which avoids the problems that I have with tinycolor, but it's a long ways from being able to be used. I'll take a look at what @aristath has to see if they'd work better together as a followup.

packages/components/src/index.js Show resolved Hide resolved
@youknowriad
Copy link
Contributor

Oh I didn't check whether this works with theme.json yet. @nosolosw would know but even if it doesn't, I'm personally fine for this being a separate concern/PR.

@ajlende ajlende merged commit 503c87f into WordPress:trunk Apr 22, 2021
@github-actions github-actions bot added this to the Gutenberg 10.6 milestone Apr 22, 2021
@jasmussen
Copy link
Contributor

🔥

@gziolo
Copy link
Member

gziolo commented May 17, 2021

Hello, this feature is amazing. Awesome work 👏🏻

I'm currently working on backporting PHP changes to WordPress core in WordPress/wordpress-develop#1257. I have two questions:

  • Are there unit tests for the duotone support?
  • There are several functions ported from TinyColor and react-color. Should we include their licenses and make it more obvious that this code is derived?

@ajlende
Copy link
Contributor Author

ajlende commented May 17, 2021

Are there unit tests for the duotone support?

No, there are not any at the moment. There was some discussion about adding some, but that discussion ended with holding off for this PR. https://github.com/WordPress/gutenberg/pull/26752/files?file-filters%5B%5D=.php#r617376509

There are several functions ported from TinyColor and react-color. Should we include their licenses and make it more obvious that this code is derived?

Nothing was ported from react-color, just TinyColor. It would be a good idea to include that license. The note in the code about react-color was to indicate that only the formats supported by react-color were copied from TinyColor, so its license doesn't need to be included.

@gziolo
Copy link
Member

gziolo commented May 17, 2021

@youknowriad added some unit tests in WordPress core for block supports in this file:

https://github.com/WordPress/wordpress-develop/blob/master/tests/phpunit/includes/testcase-block-supports.php

I'm not sure if this is the best place to add test coverage for duotone, but at least it's a good place to look at.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Block] Media & Text Affects the Media & Text Block [Feature] Media Anything that impacts the experience of managing media [Type] Feature New feature to highlight in changelogs.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants