Asset Catalog - Named Colors #68
Conversation
|
Should we add migration guide entries for the deprecated changes? |
Documentation/xcassets/swift2.md
Outdated
| @@ -21,6 +21,8 @@ You can customize some elements of this template by overriding the following par | |||
| | Parameter Name | Default Value | Description | | |||
| | -------------- | ------------- | ----------- | | |||
| | `enumName` | `Asset` | Allows you to change the name of the generated `enum` containing all assets. | | |||
| | `colorTypeName` | `ImageAsset` | Allows you to change the name of the generated color container type. | | |||
There was a problem hiding this comment.
colorTypeName defaults to… ImageAsset? Shouldn't it be s/ImageAsset/ColorAsset/?
Documentation/xcassets/swift3.md
Outdated
| @@ -20,6 +20,8 @@ You can customize some elements of this template by overriding the following par | |||
| | Parameter Name | Default Value | Description | | |||
| | -------------- | ------------- | ----------- | | |||
| | `enumName` | `Asset` | Allows you to change the name of the generated `enum` containing all assets. | | |||
| | `colorTypeName` | `ImageAsset` | Allows you to change the name of the generated color container type. | | |||
There was a problem hiding this comment.
Same typo probably here
Documentation/xcassets/swift4.md
Outdated
| @@ -20,6 +20,8 @@ You can customize some elements of this template by overriding the following par | |||
| | Parameter Name | Default Value | Description | | |||
| | -------------- | ------------- | ----------- | | |||
| | `enumName` | `Asset` | Allows you to change the name of the generated `enum` containing all assets. | | |||
| | `colorTypeName` | `ImageAsset` | Allows you to change the name of the generated color container type. | | |||
| typealias XCTAssetsType = XCTImageAsset | ||
|
|
||
| struct XCTImageAsset { | ||
| fileprivate var value: String |
There was a problem hiding this comment.
I think we should rename that property to name — even if it's fileprivate and doesn't really matter, just for consistency reasons.
Same for the XCTColorAsset of course, and same for all templates
There was a problem hiding this comment.
Seeing as we decided to keep value for the context, should I still rename this one?
| } | ||
|
|
||
| struct XCTColorAsset { | ||
| fileprivate var value: String |
There was a problem hiding this comment.
That way if we rename value to name to be more consistent, we could imagine later add properties conveying the color components as found in the JSON of the asset catalog (probably useless so maybe no need to add them for now, but we could add some variables for them in the context for people wanting to use that Parser to generate other outputs like HTML color palettes from the parsed asset catalog or whatnot…
There was a problem hiding this comment.
Actually, we can't get color components at generation time, because colors can be different on a per-device basis (and other criteria).
There was a problem hiding this comment.
Ah. Meaning one color entry in the Asset Catalog can have multiple values, because Normal vs. P3 or because they can have different actual colors for different trait collections etc, is that it?
Damn.
There was a problem hiding this comment.
Exactly, devices, memory, yadda yadda. Whatever you can use now for images also applies to colors.
There was a problem hiding this comment.
Consider the asset catalog a bit of a black box in this case. You give it a name, and it gives stuff back depending on some rules. 😄
There was a problem hiding this comment.
Allright, maybe not memory criteria, but devices + srgb and p3:
There was a problem hiding this comment.
🙀
Makes sense, but still… 😄
Ok, no color components in the generated context then ;)
There was a problem hiding this comment.
Would indeed not cost much to add an entry in the Migration Guide, even if it's only for depreciation.
Also I think we should add an Asset Catalog fixture containing both images and colors in the same catalog.
If I'm not mistaking for now the tests only check the cases of a only-images catalog and of an only-colors catalog, it's important to check that having an heterogeneous catalog won't lead to compile errors (like we could have had if we didn't split allValues in two separate properties for example)
Documentation/xcassets/swift2.md
Outdated
| @@ -21,6 +21,8 @@ You can customize some elements of this template by overriding the following par | |||
| | Parameter Name | Default Value | Description | | |||
| | -------------- | ------------- | ----------- | | |||
| | `enumName` | `Asset` | Allows you to change the name of the generated `enum` containing all assets. | | |||
| | `colorTypeName` | `ColorAsset` | Allows you to change the name of the generated color container type. | | |||
There was a problem hiding this comment.
"of the struct type representing a color"?
(I find the current wording a bit confusing wrt the difference between enumName and [color|image]TypeName otherwise)
Documentation/xcassets/swift2.md
Outdated
| @@ -21,6 +21,8 @@ You can customize some elements of this template by overriding the following par | |||
| | Parameter Name | Default Value | Description | | |||
| | -------------- | ------------- | ----------- | | |||
| | `enumName` | `Asset` | Allows you to change the name of the generated `enum` containing all assets. | | |||
| | `colorTypeName` | `ColorAsset` | Allows you to change the name of the struct type representing a color. | | |||
| | `imageTypeName` | `ImageAsset` | Allows you to change the name of the struct type representing an image. | | |||
| | `noAllValues` | N/A | Setting this parameter will disable generation of the `allValues` constant. | | |||
There was a problem hiding this comment.
Would be breaking I know, but I wonder if a sensible default wouldn't instead be to not generate allColors/allImages (as that could generate a really huge array, and in most cases for nothing as I personally never saw the need for having such an array of all your images in your whole app anyway), and thus turning that parameter to allValues instead with the inverse meaning? Thoughts?
There was a problem hiding this comment.
I don't know, it's definitely handy when you need it, and it shouldn't matter how big the generated file is. Constants as these are only allocated when accessed, so that shouldn't be an issue either.
SwiftGenKit PR: SwiftGen/SwiftGenKit#51