-
Notifications
You must be signed in to change notification settings - Fork 566
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(compose): Add Jetpack Compose format #599
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks great, thank you so much for all your work on this. I just had a minor comment on supporting outputReferences in the format.
); | ||
let allProperties; | ||
const { outputReferences } = options; | ||
const formatProperty = createPropertyFormatter({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are creating the property formatter here, but not using it in your template code. Is that intended?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops, you are definitely correct there- I had switched from using the property formatter to doing it all in the template so I could output the comment before the variable (the formatter's internals currently put it after the variable).
So I did have references working (and intended that to be the case), but forgot to move reference resolution. I'll revisit that, good catch!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Speaking of which- the current property formatter does some switching on the different CSS outputs, but I was a little hesitant to add kotlin-specific handling as well. For something like moving the comments to before a variable vs. after do you have a preference of updating the formatter or handling it in the template?
I have updated the compose format to use the formatter so reference are now getting output again. I ended up taking a middle path with the property formatter- I added support for a "none" comment style that suppresses the comment in the formatter. Adding a "kotlin" or "javadoc" comment style to the existing set of "short" and "long" didn't feel right. |
@bherbst I love the middle ground you took. I think it makes sense to me and balances the right level of flexibility in the format and formatHelper code. Great work! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
Issue #: #478
Description of changes:
Add support for Jetpack Compose output via a new
compose/object
format and associated transforms.Note: While Google is developing Compose with Android as the primary target, there is significant interest in supporting other platforms as well. JetBrains is already working with Google to support desktop applications, there is some experimental web support, and there are people who have been exploring iOS as well.
As a result, I have explicitly given the format and transforms names that do not reference Android. The only Android-specific bits in here right now are the Kotlin imports for the Compose classes and functions which are under
androidx.compose.ui
. Google has indicated they are working on better non-Android packaging so I expect this to evolve.compose/object
formatThe
compose/object
format supports outputting all token types to a Kotlinobject
.Transforms
The new
compose
transform group includes four new transforms:color/composeColor
(e.g.val backgroundRed = Color(0xFFAA0000)
)size/compose/em
(e.g.val spacingLarge = 28.em
)size/compose/remToSp
(e.g.val spacingLarge = 26.sp
)size/compose/remToDp
(e.g.val spacingLarge = 16.00.dp
)Future work:
There are a few things I'd like to improve in the future and would love feedback on:
size/compose/remToSp
transform is the only one that would need theandroidx.compose.ui.unit.sp
import). This type of communication between the format and the transform isn't really supported in Style Dictionary right now.themeable
property and/or Kotlin classes. It would be very easy to add a Kotlin class format that would support overriding a property, but I only think that would be appropriate forthemeable
properties This becomes somewhat tricky if the input contains a mix of themeable and non-themeable properties.By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.