-
Notifications
You must be signed in to change notification settings - Fork 30
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
feat: config option for math fraction digits #290
feat: config option for math fraction digits #290
Conversation
🦋 Changeset detectedLatest commit: c8e5203 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
2c9a87e
to
c8e5203
Compare
Great! I cleaned things up a bit:
Thanks for contributing! |
Excellent 👌 Thanks @jorenbroekema ! It's great to see your additions, this will be helpful for my next contribution ;) Quick question: what is your stance regarding the use of the |
Ah now that you mention it, it should probably be this: transform: (token, platform) => checkAndEvaluateMath(token, platform.mathFractionDigits), The current code also happens to work because platform options tend to be put on the config.options object, but it's not exactly its intention to use it like that, if readonly props were a thing in javascript then this would be one. |
#295 fixing it here |
[question/clarification] I'm curious the rationale for exposing register(StyleDictionary, {
platform: 'css',
mathFractionDigits: 3,
}); The higher-level question is: Where should options for See this proposed PR for more context/details on the question; would appreciate your input/feedback. Thanks! |
The answer depends on whether you want to register the option on a global level, then inside the register() function options 2nd param is appropriate, but in this case, it's a "transform" specific option, and transforms are always scoped by the platform, so I feel like putting the option in the platform config is more appropriate. Generally speaking, there is no API for passing options to transforms, the workaround for that lack of feature has always been to set options for transforms on the platform config layer. We'd need to break the API around transforms to make this more intuitive, and we didn't really get to that for the v4. I think we may also need to document this "workaround" a bit better, because it's a bit tricky for folks to figure out that transforms get the platform config passed in the transform function, and that this is how you configure transform options. |
amzn/style-dictionary#1298 this issue may be related btw. Sometimes the current SD config API is a bit awkward, because certain transforms such as the math resolver transform, can be considered "global" in the sense that they would always be applied to all platforms, meaning you're duplicating some of the stuff in your SD config for each platform, and the same thing happens to the transform options, you have to apply it to each platform. So even though registering the math minimal fraction digits may seem a lot cleaner and less awkward to put on the What I do if I have many platforms, much of which is duplicate/overlapping stuff: const basePlatform = {
mathFractionDigits: 3,
}
const sd = new StyleDictionary({
platforms: {
css: {
...basePlatform,
},
ios: {
...basePlatform,
}
}
}) |
Fixes #289
This PR adds a new platform config option called
mathFractionDigits
to control the rounding of values when resolving math.The current hardcoded value found here is
3
and one could potentially need this value to be any number.4
as it tends to make more sense when trying to convertpx
toem
andrem
, but this is debatable. We might prefer to avoid breaking changes and keep3
as default.The variable name
mathFractionDigits
was borrowed from the.toFixed()
function whose first argument isfractionDigits
to which I prependedmath
for better context in relationship with the TS transformresolveMath
. This is also to be discussed.Usage