Fix/348 modify mix value calculations #351
Conversation
|
DarioSoller
force-pushed
the
fix/348-modify-mix-value-calculations
branch
from
8a42631 to
0e7ce70
Compare
|
@jorenbroekema I came up with a fix and also added a unit and some integration tests. Please check if you have concerns and if I have been on the right direction. But I have to say that I had some issues with running the test on my machine. Either the unit or the integration tests failed. I suspected being on the wrong nodeJS version, but there is no Another possibility could be, that the package-lock.json is outdated? Here the issues I have been running into: Running the unit tests without updating the package-lock.json (integration tests run here!)
Running the integration tests with updated package-lock.json (unit tests run here!)
I would recommend, to add a Let me know, how we should proceed best in your opinion? |
|
And I have stumbled over another issue that is kinda confusing me. When these modify mix value calculations are resolved now I end up with slightly different colors than my designer in the tokens-studio Figma Plugin, even though the implementation in the plugin and the implementation in sd-transform look identical? I need more time to look into this, not sure atm, if it has to do with some rounding errors... but i already wanted to share that I could need some help on this. |
|
colorjs.io unfortunately has a couple of versions that are incompatible with certain versions of TypeScript. I've given that feedback to them to check their type interfaces, which they manually create, for compatibility with latest TypeScript + strict settings. Not sure if they made any progress on this. It's why I had colorjs.io and typescript stuck at pretty specific versions, removing the lock file may have caused those versions to change and this issue to surface again. As for the rollup-darwin-arm64 thing, yeah it's an NPM bug that surfaces for certain bundlers like Rollup and ESBuild, had it come up in a few projects already. Indeed, as suggested, removing the lockfile and nuking the node_modules solves it, but re-introduces the colorjs.io bug.. I'll try to jump in on this PR and help you fix this nuisance. I don't want to add a .nvmrc. I don't use NVM. I don't recommend NVM. I want to keep this project agnostic to it. There's an engines property in the package.json that defines compatibility with NodeJS versions, so as long as this is followed, it doesn't matter what Node version manager you end up choosing, NPM install will warn you if you're on an incompatible Node version |
|
I might have found out something regarding color difference in the Figma plugin, compared to sd-transforms output. The ColorJS.io function toString() also can take the option precision. Relating to the ColorJS.io docs the default value of this property is 5. With Probably this is the line of code in the plugin, causing this in the plugin. Once again I would really love to see more code sharing between sd-transforms and the Figma Plugin, in order to prevent these unforeseen differences. This also makes me think if my PR here has some implications for the duplicate implementation in the Figma plugin? I think the best would be that in both, sd-transforms and the Figma plugin the same precision is used, at least the ColorJS.io default precision of 5 or even more. |
|
Ok have to correct myself a little bit. As I went even further down this rabbit hole, it now seems that the implementations in the plugin are almost identical:
SD-Transforms:
I have then seen that in the Besides that I think we could still use some help in explaining, why we notice these hex color differences between the plugin and the sd-transform library!? |
|
I'm actually not so sure what's causing the differences in the plugin vs sd-transforms but I fully agree that the same implementation should be reused. This is an active topic of discussion in our team atm, I think there's a consensus for reusing but just hasn't been time yet to make it happen. As for precision, this should probably be configurable and use default of 5, by default, if that's the one that ColorJS uses. But let's tackle this separately. |
jorenbroekema
force-pushed
the
fix/348-modify-mix-value-calculations
branch
from
205c8ab to
76dbfba
Compare
|
Latest colorjs.io works fine with latest typescript, just not with ts-node. I decided to migrate the integration tests away from mocha to vitest. Things are a bit simpler that way in terms of setup. This upgrade to latest colorjs.io may also fix some small issues, I see that the precision of 3 now actually works to be 3 decimals instead of 2, this may already affect your hex values slightly. Let me know if it's okay to merge this |
|
@jorenbroekema Actually I was just working on a backwards compatible way of configuring the color calculation math fraction digits and the color precision setting on different levels (platform config, transform options, or on individual token basis). Let me checkout your update and I will try to bring my adjustments in the tests on the new testing setup. Then i will explain in detail what my thoughts have been and if you are convinced to merge this, or if we just pick parts of it. Thanks for taking care of the dependency updates. |
|
@jorenbroekema in the meantime, could you think about how the displayed hex color in the Figma plugin for a mix modifier, can be guaranteed to be the value that is actually coming out of the sd-transforms? Otherwise I fear that this will always create confusion for people (incl. myself). I guess this could be solved by simply increasing the the mathFractionDigits and color precision defaults. If someone wants to have less precision by configuring it accordingly in sd-transforms, it should be more intuitive, why the values to the Figma plugin differ, than the other way around. |
DarioSoller
force-pushed
the
fix/348-modify-mix-value-calculations
branch
from
7977955 to
9d40322
Compare
|
CI/CD Typescript errors can be solved with PR: tokens-studio/types#49 |
There was a problem hiding this comment.
@jorenbroekema I add a few comments in the code. Things we should consider and think about, as well as explanations of the code changes.
| @@ -36,7 +36,7 @@ | |||
| "@bundled-es-modules/deepmerge": "^4.3.1", | |||
| "@bundled-es-modules/postcss-calc-ast-parser": "^0.1.6", | |||
| "@tokens-studio/types": "^0.5.1", | |||
There was a problem hiding this comment.
We will have to update the types, after my PR for the @tokens-studio/types is merged.
| ): Color { | ||
| const mixValue = Math.max(0, Math.min(1, Number(amount))); | ||
|
|
||
| return new Color(color.mix(mixColor, mixValue, { space: colorSpace }).toString()); | ||
| return new Color(color.mix(mixColor, mixValue, { space: colorSpace }).toString({ precision })); |
There was a problem hiding this comment.
taking the actual configured color precision, instead of always falling back to the colorJS.io default precision.
There was a problem hiding this comment.
I think we should use a single property here, let's reuse mathFractionDigits as well here. They are kinda the same thing, amount of digits after the decimal point for numbers. Specifying a different mathFractionDigits for potential math expressions in color definitions and then also a different precision for the final output doesn't really make sense to me to be honest.
There was a problem hiding this comment.
Just be aware, that you then have a coupling between spacing calculations and color calculations, especially in the color modifiers. I am not 100% sure, if this coupling makes sense? More details in this discussion. But I agree, that it would make things much simpler in the code.
| let resolvedMathFractionDigits: number = defaultFractionDigits; | ||
| if (modifier?.mathFractionDigits) { | ||
| resolvedMathFractionDigits = modifier.mathFractionDigits; | ||
| } |
There was a problem hiding this comment.
Actually modifier.mathFractionDigits will never be undefined here, but I had to initialze resolvedMathFractionDigits that way, because otherwise I would have needed to change the ColorModifyOptions type in such a way, that mathFractionDigits is not optional. More elegant ways of initialization stumbled over 100% code coverage in the tests. Because of backwards compatibility, I think it's fine like this then?
| let resolvedColorPrecision: number = defaultColorPrecision; | ||
| if (modifier?.precision) { | ||
| resolvedColorPrecision = modifier.precision; | ||
| } |
There was a problem hiding this comment.
Same here: Actually modifier. precision will never be undefined here, but I had to initialze resolvedColorPrecision that way, because otherwise I would have needed to change the ColorModifyOptions type in such a way, that precision is not optional. More elegant ways of initialization stumbled over 100% code coverage in the tests. Because of backwards compatibility, I think it's fine like this then?
There was a problem hiding this comment.
see other comment, I can look into it.
| } | ||
| if (!modifier.precision) { | ||
| modifier.precision = options?.precision ?? defaultColorPrecision; | ||
| } |
There was a problem hiding this comment.
If mathFractionDigits or precision are not set on the token level modify, then either use the respective configuration coming from the platform config or the transform options, or fall back to the default values for mathFractionDigits and precision.
There was a problem hiding this comment.
I wonder if we should just pass the "options" to modifyColor function, and handle this logic in there. Since there it already has logic that deals with the fallback values. So then it would just be:
const resolvedMathFractionDigits = modifier.mathFractionDigits ?? options?.mathFractionDigits ?? defaultFractionDigits;
There was a problem hiding this comment.
I will pick this topic up again, as soon as we know more, in which direction ColorJS.io is heading now.
There was a problem hiding this comment.
It would seem to me that we should max out precision for now and then constrain the amount of digits after the decimal period based on mathFractionDigits option that's also used for the resolve math transform. This essentially adds the precision, in the intuitive manner. It feels to me that ColorJS.io will not end up agreeing with my stance on precision, and I'm not that interested in creating a PR for adding a 2nd option if it means that I'm just going to end up turning off precision 😅 I can already do that now.
There was a problem hiding this comment.
Ah sigh, I just realized during the color modify, it's kinda hard to adjust the amount of decimals, because values are usually strings of some kind of color format... So I guess we do need to either park it until the colorjs.io work is done or we need to just pick a sensible precision for now like 5, and call it a bugfix..
| } | ||
|
|
||
| return resolvedOpts; | ||
| }; |
There was a problem hiding this comment.
Might have room for improvement, if you have an idea?
First check if mathFractionDigits or precision are set in the platform config. If they are also set in the transform options, then the platform configs get overwritten. So it also supports the case that either one of them is set on the platform level and on the transform option level.
As we now have precision also in the platform level, I think I have to make a PR for updating the platform types in Style-Dictionary? If so it might actually be better to rename precision to colorPrecision, so that it is clear that this only alters the precision with which colors are represented in the output files?
There was a problem hiding this comment.
Yeah I'll take a look at making this a bit more elegant.
I think to summarize we have this order of precedence, from highest prio to least:
- Token definition -> modify -> precision
- Platform config -> precision
- SD-transforms
register(SD, options)-> options -> transformName -> precision - default values if none of the above is defined
In v6 I want to be able to pass transform options a bit differently, similar to how you do with formats. Then in the above list we can remove 2 and 3 and instead we use the new method of passing transform options.
There was a problem hiding this comment.
Yeah right. As I am reading your comment, I might have assumed another order of priority like:
- Token definition -> modify -> precision
- SD-transforms register(SD, options) -> options -> transformName -> precision
- Platform config -> precision
- default values if none of the above is defined
But your's makes definitely more sense, as I am thinking of it. Little adjustment needed then in the code: register.ts > resolveColorModifyTransformOpts()
There was a problem hiding this comment.
I corrected the order accordingly.
DarioSoller
force-pushed
the
fix/348-modify-mix-value-calculations
branch
from
3ca6a91 to
fae40dd
Compare
DarioSoller
force-pushed
the
fix/348-modify-mix-value-calculations
branch
from
fae40dd to
33b6234
Compare
… more platform config integration tests
DarioSoller
force-pushed
the
fix/348-modify-mix-value-calculations
branch
from
33b6234 to
a349914
Compare
| let resolvedMathFractionDigits: number = defaultFractionDigits; | ||
| if (modifier?.mathFractionDigits) { | ||
| resolvedMathFractionDigits = modifier.mathFractionDigits; | ||
| } |
There was a problem hiding this comment.
| let resolvedMathFractionDigits: number = defaultFractionDigits; | |
| if (modifier?.mathFractionDigits) { | |
| resolvedMathFractionDigits = modifier.mathFractionDigits; | |
| } | |
| const resolvedMathFractionDigits = modifier.mathFractionDigits ?? defaultFractionDigits; |
If coverage complains then I think we need to add a test to cover the branch that is not covered.
If typescript complains because modifier can be undefined then we need to fix that because it can't be undefined. I'd be really surprised if it thinks it can be, it's not an optional parameter. I can look into it more deeply if it's an issue.
|
|
||
| // 3 for backwards compatibility, but should better be 5, referring to the default precision (5) of the colorJS.io library | ||
| // see: https://colorjs.io/docs/output.html#getting-a-string-representation-of-a-color | ||
| export const defaultColorPrecision = 3; |
There was a problem hiding this comment.
If we now decide to just stick with the defaultColorPrecision of 5, I would recommend, that we still explicitly set this precision, instead of relying on the ColorJS default. As this precision setting has quite some unintuitive impact on (mixed) colors, I would really define it explicitly. Let me know, if the place where I have put constants.ts makes sense for you?
|
Closed in favour of PR #354 |
Fixes #348 and might enhance possible calculation resolution generically for all modifier properties, as suggested in the issue discussion