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

Format errors in some Android transforms? (Style Dictionary + sd-transforms) #1132

Open
nhoizey opened this issue Mar 25, 2024 · 11 comments
Open

Comments

@nhoizey
Copy link
Contributor

nhoizey commented Mar 25, 2024

I'm trying to use style-dictionary to output XML files for Android.

My tokens are exported from Tokens Studio, and I'm using sd-transforms.

I didn't know if I had to open the issue here or in the other repo.

Only some of the tokens are generated with the right type. Some tokens are generated with a wrong type. Some are not generated at all.

I tried previously with older versions of both packages, now with the latest ones:

  • sd-transforms 0.15.0
  • style-dictionary 4.0.0-prerelease.19

My platforms configuration now starts with this:

transformGroup: 'tokens-studio',
transforms: [...StyleDictionary.transformGroup.android],

Is that how it's supposed to work since sd-transforms v0.15.0?


I tried with the following configuration, as (quoted from docs) "it is recommended to use the android/resources format with a custom filter instead of" specific android/integers, android/dimens, android/colors, etc.

{
  destination: 'resources.xml',
  format: 'android/resources',
},

The list of resource types for Android I want to be able to generate for projects is detailed in files located in the values/ folder presented in this documentation.

The XML file I get contains:

Tokens Studio type Output XML type Expected type
color <color> <color>
boxShadow <string> <string>
typography <string> <string>
lineHeights <string> <dimen>
opacity <string> <integer>
sizing <string> <dimen>
borderWidth <string> <dimen>
borderRadius <string> <dimen>
fontSizes <string> <dimen>
spacing <string> <dimen>
number with a decimal value <integer> 🤔 <dimen>

I guess there's something wrong in the sequence of transformations.

For example, is the sizing type from Tokens Studio transformed into the size type expected by Style Dictionary?


How can I check the types in every token between transformations done by sd-transforms and style-dictionary?

@nhoizey
Copy link
Contributor Author

nhoizey commented Mar 25, 2024

I guess this might be related to tokens-studio/sd-transforms#134

@jorenbroekema
Copy link
Collaborator

Is that how it's supposed to work since sd-transforms v0.15.0?

Pretty much, although the default tokens-studio transformGroup contains CSS specific transforms, which may give unexpected values if your target is android

For example, is the sizing type from Tokens Studio transformed into the size type expected by Style Dictionary?

I made some efforts to get rid of determining the token type from the CTI token structure and instead look at the token.type property. I think the built-in format for android checks for token.type === 'dimension', assuming DTCG format. You may need a custom one that also matches for the types from Tokens Studio. I think it may also be okay to adopt those types to Style-Dictionary but I want to discuss that with the other maintainers.

I guess this might be related to tokens-studio/sd-transforms#134

Yes some of it might be if the values end up incorrectly, if they are outputted to the incorrect android xml tags, then that is more of an issue with the built-in android format in style-dictionary.

Would you be open to create an integration test in the integration folder on the v4 branch showing what your token input is of all these types and what the expected android xml file is for this? Then we can work to close the gaps and make it green

@nhoizey
Copy link
Contributor Author

nhoizey commented Apr 2, 2024

@jorenbroekema I will try to create integration tests. Should I modify the existing integration/android.test.js or create another one?

Also, can I had new tokens to __integration__/tokens/ or will it make other tests break?

@jorenbroekema
Copy link
Collaborator

@jorenbroekema I will try to create integration tests. Should I modify the existing integration/android.test.js or create another one?

Probably a bit easier to create a separate one, if you use snapshots it'll then have it's own snapshot file which might make it a bit easier to work with. But I'm fine with it either way.

Also, can I had new tokens to __integration__/tokens/ or will it make other tests break?

It will probably break other tests, maybe use a totally separate token file and we can integrate it into the integration tokens afterwards, it will require updating a lot of snapshots but I can help with that.

@nhoizey
Copy link
Contributor Author

nhoizey commented Apr 2, 2024

Probably a bit easier to create a separate one, if you use snapshots it'll then have it's own snapshot file which might make it a bit easier to work with. But I'm fine with it either way.

Ok. So I should write both the test and the expected snapshot, right?

It will probably break other tests, maybe use a totally separate token file and we can integrate it into the integration tokens afterwards, it will require updating a lot of snapshots but I can help with that.

Do you suggest that I create a new /__android_tests__/ folder (that could be moved later), so the default script npm run test:node doesn't use it?

I can create /__android_tests__/tokens/*.json and /__android_tests__/android.test.js.

Sorry if I'm asking silly questions, I want to help but it's pretty obscure for me… 😅

@jorenbroekema
Copy link
Collaborator

jorenbroekema commented Apr 2, 2024

Ok. So I should write both the test and the expected snapshot, right?

No, the first time you run the test with matchSnapshot call, it will initialize the snapshot for you, then if you intend for that snapshot to be different you can hand-edit it, then the test will go to red when it's no longer matching the snapshot. Then we can work towards fixing the source code to make sure it matches the snapshot as you intend it to, and make the test green again.

You can create a new file __integration__/android-2.test.js for now, then inside that test use source: ['__integration__/android-2.tokens.json'] and create a file __integration__/android-2.tokens.json, or something like that. Not a problem if the files look a little weird or the names are weird, we can make it nice after we fix things ^^

@nhoizey
Copy link
Contributor Author

nhoizey commented Apr 2, 2024

Ok, thanks for your guidance! 🙏

Additionally, is there a recommended way to track transforms in both sd-transforms and Style Dictionary? For example, looking at both code bases, I couldn't find where the fontSizes (plural) type from Tokens Studio becomes a fontSize (singular) type in Style Dictionary.

@jorenbroekema
Copy link
Collaborator

I couldn't find where the fontSizes (plural) type from Tokens Studio becomes a fontSize (singular) type in Style Dictionary.

Hah well that's probably because that doesn't happen at all, I'm pretty sure Tokens Studio will eventually do a breaking change and change the type to singular, because the DTCG spec also uses singular form, and Tokens Studio doing plural form is more of an unintended oversight than a conscious decision, it's definitely a flaw/inconsistency there.

@nhoizey
Copy link
Contributor Author

nhoizey commented Apr 2, 2024

Hah well that's probably because that doesn't happen at all, I'm pretty sure Tokens Studio will eventually do a breaking change and change the type to singular

Oh, good to know, thanks!

Should I open an issue on sd-transforms side to track this?

@jorenbroekema
Copy link
Collaborator

Yeah sure thing, good idea!

@nhoizey
Copy link
Contributor Author

nhoizey commented Jun 29, 2024

@jorenbroekema I see that you closed my PR #1173

How do you think we should address this issue now? How can I help?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants