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.

Sign up for GitHub

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 colord package to block editor; Replace tinycolor2 with colord on duotone #34603

Merged
merged 1 commit into from
Sep 27, 2021
Merged

Add colord package to block editor; Replace tinycolor2 with colord on duotone #34603

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 8 additions & 6 deletions packages/block-editor/src/hooks/duotone.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,8 @@
* External dependencies
*/
import classnames from 'classnames';
import tinycolor from 'tinycolor2';
import { colord, extend } from 'colord';
import namesPlugin from 'colord/plugins/names';

/**
* WordPress dependencies
Expand All @@ -25,6 +26,8 @@ import BlockList from '../components/block-list';

const EMPTY_ARRAY = [];

extend( [ namesPlugin ] );

/**
* Convert a list of colors to an object of R, G, and B values.
*
Expand All @@ -36,11 +39,10 @@ export function getValuesFromColors( colors = [] ) {
const values = { r: [], g: [], b: [] };

colors.forEach( ( color ) => {
// Access values directly to skip extra rounding that tinycolor.toRgb() does.
Copy link
Member Author

Choose a reason for hiding this comment

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

Hi @ajlende, would you be able to check if this change is ok? Also if you have additional insights on why tinycolor.toRgb() was not a possibility before (in order to make sure we are not doing a mistake by calling colord.toRgb) it may be helpful. I checked that tinycolor.toRgb returns an object with {r,g,b} with values between 0 and 255 without any rounding it seems to access the internal tinycolor values is equivalent but I may be missing something.

Copy link
Contributor

Choose a reason for hiding this comment

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

One of TinyColor's quirks is that it rounds values at random points within its calculations—sometimes at this point it's already rounded once and sometimes it isn't rounded yet. The browser rounds values used in CSS when displaying values, so it's usually not a problem. But SVG accepts floating point numbers for its color values and is doing a color operation on them, so it's better to skip as much intermediate rounding as possible.

It looks like colord also rounds the values in toRgb(), so if there's a way to skip that, it would probably be better. If not, it's not that big of a deal. Visually, you're probably not going to notice; just when using a color picker, the color values might be off by one in some edge cases.

TinyColor has a lot of quirks—I had to port it, along with all the quirks, to PHP for the SVG rendering there, so we'll also want to update lib/duotone.php too to match the behavior of colord.

Copy link
Contributor

Choose a reason for hiding this comment

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

With switching the PHP over, @aristath has a library that we talked about during the initial duotone implementation that we may be able to switch over if it behaves the same as colord. #26752 (comment)

Copy link
Member

Choose a reason for hiding this comment

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

Please let me know if we want to do something like that... I can definitely port colord to PHP or tweak the existing library as needed.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think in practice the rounding never happens on our use cases the colors most of the time come in hex "#ff0023" and so each RGB value is really an integer calling round on an integer returns the same number. I guess the case where we may have a difference is on the alpha part of the rgba but when dealing with alpha's the color that appears on the color picker is not predictable so having a unit difference on that case is ok I guess.

const tcolor = tinycolor( color );
values.r.push( tcolor._r / 255 );
values.g.push( tcolor._g / 255 );
values.b.push( tcolor._b / 255 );
const rgbColor = colord( color ).toRgb();
values.r.push( rgbColor.r / 255 );
values.g.push( rgbColor.g / 255 );
values.b.push( rgbColor.b / 255 );
} );

return values;
Expand Down