-
Notifications
You must be signed in to change notification settings - Fork 567
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
Adding common transforms for Swift in iOS #255
Conversation
This is awesome! I will take a look this week, but thank you so much for doing this! |
👍 |
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.
WTF I thought I submitted a review already.. oh well.. Anyways, great job on this PR! Looks great, I just have a few minor comments
Review comments addressed, added |
Hey, this is GREAT! We have wanted to add Swift support for awhile (I'm pretty amateur w/ Swift), so thank you for doing this. I do have one concern; the setup appears to require the user to output all sizes in one file and all other properties in another. It requires this decision making to happen in the config. Instead, it would be great if there was only a single Swift format and the properties themselves were used to determine when/how to add in type information. Maybe we create a new transform to add an attribute containing the token type (tokenType? classType?). Then when the formatter is run, it can use that information to output the type info correctly. Does that make sense? Or am I misinterpreting something about Swift? |
Honestly, that's a good point. I had considered that, with the way "autocomplete/intellisense" works in XCode, that having to type It's the transform groups where we essentially "decide" the conventions for the language, so I'll play with it and get back to you. |
I've added the default |
My apologies for the lateness, I am currently in Europe right now. I will dig into the code tonight (in about 12 hours), but reading over your comment and description it sounds good! Thank you again for all your work on this. |
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.
2 super minor things, after that I'm good to go. Thank you again for your work on this!
examples/basic/config.json
Outdated
"destination": "StyleDictionary.swift", | ||
"format": "ios-swift/class.swift", | ||
"className": "StyleDictionary", | ||
"type": "StyleDictionaryColorName", |
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.
It still doesn't look like 'type' here is being used, I can't see it in the format code. Is this meant to do something or could we remove it?
// Filter to only those props wanted based on the filter, then sort | ||
// them by category so we keep like props together, then by name | ||
// so they are easier to find alphabetically. | ||
var filteredProps = _.filter(allProperties, this.filter); |
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.
I'm 99% sure you can remove this, I believe when we properly added the filter functionality we actually run the filters before the format so the format code itself doesn't have to worry about implementing filters. Take a look here
@@ -578,7 +628,7 @@ module.exports = { | |||
return prop.attributes.category === 'content'; | |||
}, | |||
transformer: function(prop) { | |||
return '\'' + prop.value + '\''; | |||
return wrapValueWith('\'', prop); |
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.
Love this addition!
Thanks! Will make the updates and get back to you. Thanks again!
…On Sat, Mar 23, 2019 at 2:07 PM Danny Banks ***@***.***> wrote:
***@***.**** commented on this pull request.
2 super minor things, after that I'm good to go. Thank you again for your
work on this!
------------------------------
In examples/basic/config.json
<#255 (comment)>:
> @@ -64,6 +64,42 @@
}
}
}]
+ },
+ "ios-swift": {
+ "transformGroup": "ios-swift",
+ "buildPath": "build/ios-swift/",
+ "files": [{
+ "destination": "StyleDictionary.swift",
+ "format": "ios-swift/class.swift",
+ "className": "StyleDictionary",
+ "type": "StyleDictionaryColorName",
It still doesn't look like 'type' here is being used, I can't see it in
the format code. Is this meant to do something or could we remove it?
------------------------------
In lib/common/templates/ios-swift/enum.swift.template
<#255 (comment)>:
> +// <%= this.destination %>
+//
+<%
+ // for backward compatibility we need to have the user explicitly hide it
+ var showFileHeader = (this.options && this.options.hasOwnProperty('showFileHeader')) ? this.options.showFileHeader : true;
+ if(showFileHeader) {
+ print("// Do not edit directly\n");
+ print("// Generated on " + new Date().toUTCString());
+ }
+%>
+//
+<%
+ // Filter to only those props wanted based on the filter, then sort
+ // them by category so we keep like props together, then by name
+ // so they are easier to find alphabetically.
+ var filteredProps = _.filter(allProperties, this.filter);
I'm 99% sure you can remove this, I believe when we properly added the
filter functionality we actually run the filters before the format so the
format code itself doesn't have to worry about implementing filters. Take
a look here
<https://github.com/amzn/style-dictionary/blob/master/lib/buildFile.js#L45>
------------------------------
In lib/common/transforms.js
<#255 (comment)>:
> @@ -578,7 +628,7 @@ module.exports = {
return prop.attributes.category === 'content';
},
transformer: function(prop) {
- return '\'' + prop.value + '\'';
+ return wrapValueWith('\'', prop);
Love this addition!
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#255 (review)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAObpWV4Wbc8WSPcKQD-A5FJgSfZSPD-ks5vZpeogaJpZM4bnl85>
.
--
*when you don’t create things, you become defined by your tastes rather
than ability. your tastes only narrow & exclude people. so create.*
- _why
|
@tchype Thank again for all your hard work on this. We would love to get this functionality merged in for everyone to use - let me know if we can do anything to help |
Sorry -- thought I finished this. Will finish today @chazzmoney ! |
Ready for review/merge @chazzmoney @dbanksdesign . Thanks! |
LGTM! |
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!
Based on swift implementation ( #255 ), it adds swift transforms, transformGroups, formats, and example: * flutter/class.dart format * flutter and flutter-separate transformGroups * color/hex8flutter, content/flutter/literal, asset/flutter/literal, font/flutter/literal, and size/flutter/remToDouble transforms * advanced/flutter example fixes #288 Co-authored-by: MDemetrio <[email protected]> Co-authored-by: Danny Banks <[email protected]>
Based on swift implementation ( #255 ), it adds swift transforms, transformGroups, formats, and example: * flutter/class.dart format * flutter and flutter-separate transformGroups * color/hex8flutter, content/flutter/literal, asset/flutter/literal, font/flutter/literal, and size/flutter/remToDouble transforms * advanced/flutter example fixes #288 Co-authored-by: MDemetrio <[email protected]> Co-authored-by: Danny Banks <[email protected]>
Issue #, if available: #161
Description of changes:
Adding ability to use
ios-swift
andios-swift-separate
transform groups. Examples:ios-swift: all in one dictionary class file
ios-swift-separate: allow enum-looking use so each category is separated into different files
For
ios-swift-separate
(as shown above), it creates public enums but adds static members to the enum class without anycase
statements. If we were to type the enum and then set values, every time someone wants to use them they would have to do something like:By doing it the way we do in the proposed changes, we allow for:
Also added the following transforms:
name/ti/camel
: supports camel-case without the leading category (Swift encourages succinct names); also added supporting tests.size/swift/remToCGFloat
: like the already-in-placecolor/UIColorSwift
, this adds support forCGFloat
, which is used in UIKit for decimal values; swift will view a literal number with decimal point values as aDouble
without a class initializer.content/swift/literal
: thecontent/objC/literal
prepends the@
; swift doesn't use this for literals.font/swift/literal
: thefont/objC/literal
prepends the@
; swift doesn't use this for literals.asset/swift/literal
: theasset/objC/literal
prepends the@
; swift doesn't use this for literals.Added the
ios-swift
andios-swift-separate
transform groups using the new transforms as appropriate.Added support for two specific templates:
ios-swift/class.swift
: leverages Swift's type inference to allow any literal that is supported (e.g., String, UIColor, CGFloat) to be added as a property to a class.ios-swift/enum.swift
: leverages Swift's type inference to allow any literal that is supported (e.g., String, UIColor, CGFloat) but puts it into an enum instead of a class.Updated the
basic
example to show size and color to match the Objective-C version by leveragingios-swift-separate
and also added a singleios-swift
output to show how they can all be used in oneStyleDictionary
class instead.By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.