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

feat: add configuration for android jetpack compose output #223

Draft
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

felix-ico
Copy link
Collaborator

@felix-ico felix-ico commented Aug 2, 2023

this generates a .kt file to be used in jetpack compose. There are still a couple of issues, namely:

  • it seems like colors with an alpha value get not converted, for example:
    val TelekomColorUiBorderDisabled = [object Object]

  • some values do not get converted correctly, for example:
    val TelekomColorTextAndIconOnSubtleCyan = #00738A
    should be
    val TelekomColorTextAndIconOnSubtleCyan = Color(0x00738A00)

some open questions:

  • should the light and dark files be bundled, like we do with design-tokens-all.css?
  • any other values that should be ported?

Copy link
Collaborator

@acstll acstll left a comment

Choose a reason for hiding this comment

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

A couple of comments.

Also remember you can run npm run format 🤓

config/shared.js Outdated
@@ -133,6 +133,18 @@ StyleDictionary.registerTransform({
transformer: StyleDictionary.transform['color/css'].transformer,
});

StyleDictionary.registerTransform({
Copy link
Collaborator

Choose a reason for hiding this comment

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

This register call could live in compose.config.js since it's actually not shared with other configs.

Comment on lines 59 to 63
{
const formatted = token.path.map(el => format(el))
token.name = `Telekom${formatted.toString().replace(/,/g, '')}`
return token
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

This looks like this filter function is being misused (it works nevertheless). The filter function should only do the checks and return a boolean.

For formatting the name you could register a custom tranform of type "name". I think it works the same was as value transform, you return the new name instead of setting it in the token object.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

thanks i was still a bit unsure about when/where to use transformers vs filters (even though the name should give it away)

@acstll
Copy link
Collaborator

acstll commented Aug 4, 2023

  • it seems like colors with an alpha value get not converted, for example:
    val TelekomColorUiBorderDisabled = [object Object]

they get converted but not fully, [object Object] is rendered because the value is actually { color: 'Color(0xff000000)', alpha: 0.21 }. I tried using the custom color/alpha transform we already have but I couldn't get it to work, and it gets tricky with modes.

We could either add an extra transform (imagine outputting some Kotlin code where the alpha gets applied, don't know how that might look). Or (I prefer this), overwrite the built-in color/composeColor transform (like you actually tried here) to handle the alpha modifier directly. (You probably saw already the original transform is really simple, we use the same tinycolor2 dep…)

  • some values do not get converted correctly, for example:
    val TelekomColorTextAndIconOnSubtleCyan = #00738A
    should be
    val TelekomColorTextAndIconOnSubtleCyan = Color(0x00738A00)

there are 4 colors in the semantic layer that are hard-coded, ideally the color scale should be updated (change the colors in core), but until then we might need to handle this exceptionally (in a custom transform?) (see SCL-296)

  • should the light and dark files be bundled, like we do with design-tokens-all.css?
  • any other values that should be ported?

this I don't know haha, it depends on how these things are handled in such an environment like Compose, we need to figure it out

@felix-ico
Copy link
Collaborator Author

felix-ico commented Aug 4, 2023

they get converted but not fully, [object Object] is rendered because the value is actually { color: 'Color(0xff000000)', alpha: 0.21 }. I tried using the custom color/alpha transform we already have but I couldn't get it to work, and it gets tricky with modes.

We could either add an extra transform (imagine outputting some Kotlin code where the alpha gets applied, don't know how that might look). Or (I prefer this), overwrite the built-in color/composeColor transform (like you actually tried here) to handle the alpha modifier directly. (You probably saw already the original transform is really simple, we use the same tinycolor2 dep…)

Yup was already trying out something like your second, preferred, suggestion.

there are 4 colors in the semantic layer that are hard-coded, ideally the color scale should be updated (change the colors in core), but until then we might need to handle this exceptionally (in a custom transform?) (see SCL-296)

this is good to know 🙏

should the light and dark files be bundled, like we do with design-tokens-all.css?

In the jetpack compose starter project there is some light/dark mode configuration apparently, i did not look into it in detail but I think I can get some information on implementation there.

any other values that should be ported?

looking here https://github.com/amzn/style-dictionary/blob/main/lib/common/transforms.js i guess font sizes and possibly spacings could also be ported (although not sure if what we have currently would be ok for mobile apps 🤔)

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

Successfully merging this pull request may close these issues.

2 participants