Add colord package to block editor; Replace tinycolor2 with colord on duotone #34603
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jorgefilipecosta
added
the
[Type] Code Quality
jorgefilipecosta
commented
Sep 6, 2021
| @@ -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. | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
|
Size Change: +12 B (0%) Total Size: 1.07 MB
ℹ️ View Unchanged
|
jorgefilipecosta
force-pushed
the
add/colord-package-block-editor-replace-tinycolor2-with-colord-on-duotone-hook
branch
from
0942d43
to
84d0a2d
Compare
Based on these assumptions I guess we can merge this PR. |
jorgefilipecosta
deleted the
add/colord-package-block-editor-replace-tinycolor2-with-colord-on-duotone-hook
branch
Merged
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Part of #34286.
This PR replaces the tinycolor2 library with colord on the duotone hook.
How has this been tested?
I added some duotone filters to image with different color configurations and verified things still work.