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

Update to Style Dictionary v4 #203

Merged
merged 16 commits into from
Sep 20, 2024
Merged

Update to Style Dictionary v4 #203

merged 16 commits into from
Sep 20, 2024

Conversation

c4rlosviteri
Copy link
Contributor

@c4rlosviteri c4rlosviteri commented Aug 14, 2024

This PR includes an update to SD v4 and its several configurations.

Migration guideline: https://styledictionary.com/version-4/migration/

<dimen name="cdr_text_default_subheadline_line_spacing">20.00dp</dimen>
<dimen name="cdr_text_default_body1_size">15.00sp</dimen>
Copy link
Member

Choose a reason for hiding this comment

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

is size intended to be added to the output?

package.json Outdated
@@ -1,6 +1,7 @@
{
"name": "@rei/cdr-tokens",
"version": "12.2.0",
"version": "12.3.0",
Copy link
Member

Choose a reason for hiding this comment

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

are we expecting any breaking changes with this update?
https://rei-coop.atlassian.net/wiki/spaces/TP/pages/31031923/Cedar+Release+Checklist

import { colorCssTransitive } from './transforms/color/color-css-transitive.mjs'
import { hex8AndroidTransitive } from './transforms/color/hex8-android-transitive.mjs'
import { uiColorTransitive } from './transforms/color/uicolor-transitive.mjs'
import { spTransitive } from './transforms/size/sp-transitive.mjs'
Copy link
Member

Choose a reason for hiding this comment

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

Is this new?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, for now to support token.attribute.category

import { js as jsFormat } from './formats/js.mjs'
import { site } from './formats/site.mjs'
import { figma as figmaFormat } from './formats/figma.mjs'
import { androidColors, androidDimens, androidFontDimens } from './formats/android.mjs'
Copy link
Member

Choose a reason for hiding this comment

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

where are these coming from?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

From the SD v4 source code, just changing the code a bit to support token.attributes.category

https://github.com/amzn/style-dictionary/blob/main/lib/common/formats.js

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Most of this code is gonna be removed when the tokens are updated

Copy link
Member

Choose a reason for hiding this comment

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

Are you planning to update the tokens as a follow on of as part of this PR?

@c4rlosviteri c4rlosviteri force-pushed the pr/update-to-sd-v4 branch 4 times, most recently from 19ec846 to 6eb3850 Compare August 19, 2024 17:42
README.md Outdated
value # *required* The token value (most token values should be referenced from options)
category # *required* The tokens category (used to transform values for their specific platform)
$value # *required* The token value (most token values should be referenced from options)
$type # *required* The tokens category (used to transform values for their specific platform)
Copy link
Member

Choose a reason for hiding this comment

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

"# required The tokens category"
to
"# required The tokens type"

README.md Outdated
@@ -130,13 +130,13 @@ Token output of above:

Categories define how style-dictionary should transform values between platforms.

For example, a category of "size" will transform to 'rem' for SCSS/LESS but 'dp' for Android. A category of "font-size" will still transform values to 'rem' for SCSS/LESS but 'sp' for Android.
For example, a category of "size" will transform to 'rem' for SCSS/LESS but 'dp' for Android. A category of "fontSize" will still transform values to 'rem' for SCSS/LESS but 'sp' for Android.
Copy link
Member

Choose a reason for hiding this comment

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

category to type

@@ -185,19 +185,19 @@ docs: {

Deprecated tokens should be moved to a seprate file (or into the existing file) which corresponds to the release cycle in which they will be deprecated.

For example, if tokens will be considered deprecated in the "Winter 2019" release they would be moved into a file called `deprecated-2019-winter.json5` in whichever directory they currently reside. Structure for naming the file is : `deprecated-<year>-<release>`
For example, if tokens will be considered deprecated in the "Winter 2019" release they would be moved into a file called `deprecated-2024-summer.json5` in whichever directory they currently reside. Structure for naming the file is : `deprecated-<year>-<release>`
Copy link
Member

Choose a reason for hiding this comment

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

"Q4 2024"
deprecated-2024-Q2.json

README.md Outdated
'on-dark': {
value: '{options.color.heart-of-darkness}',
category: 'color',
'deprecated-2019-winter': { // <-------- `deprecated-<year>-<release>`
Copy link
Member

Choose a reason for hiding this comment

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

"deprecated-2024-Q4"

--cdr-text-heading-serif-strong-900-size: 3.6rem;
--cdr-text-heading-serif-strong-900-height: 4.4rem;
--cdr-text-heading-serif-strong-1000-family: Stuart, "Stuart fallback", Georgia, serif;
--cdr-text-heading-serif-strong-1000-style: normal;
--cdr-text-heading-serif-strong-1000-weight: 600;
--cdr-text-heading-serif-strong-1000-spacing: 0px;
--cdr-text-heading-serif-strong-1000-spacing: 0;
Copy link
Member

Choose a reason for hiding this comment

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

this makes me feel that there should just be a much smaller heading spacing token
--cdr-text-heading-spacing: 0;

"filePath": "tokens/web/text-heading.json5",
"$extensions": {
"studio.tokens": {
"originalType": "letterSpacing"
Copy link
Member

Choose a reason for hiding this comment

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

this allows it to map back?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes

}
},
"note": {
"value": "#c4b03b",
"type": "color"
"$value": "hsl(51, 54%, 50%)",
Copy link
Member

Choose a reason for hiding this comment

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

why are these converting to HSL?

grid.mjs Outdated
'background-colors': formattedTokens
}

console.log(`https://contrast-grid.eightshapes.com/?${queryString.stringify(urlData)}`)
Copy link
Member

Choose a reason for hiding this comment

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

It would be super helpful to

  • add documentation on confluence that informs design and product on how to access this chart
  • add a GHPages directory and publish this to it so that its always available

}
},
{
"value": "rgba(3, 3, 1, 0.9)",
"$value": "rgba(3, 3, 1, 0.9)",
"$type": "color",
"alpha": 0.9,
Copy link
Member

Choose a reason for hiding this comment

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

do these alpha nodes do anything at this point? it looks like the alpha is included in the rgba

"alpha": 0.75,
"category": "color",
"$value": "rgba(12, 11, 8, 0.75)",
"alpha": ".75,",
Copy link
Member

Choose a reason for hiding this comment

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

I dont think the alpha should be a string?

@c4rlosviteri c4rlosviteri force-pushed the pr/update-to-sd-v4 branch 5 times, most recently from 0bafb87 to c41383c Compare September 6, 2024 17:54
@c4rlosviteri c4rlosviteri force-pushed the pr/update-to-sd-v4 branch 2 times, most recently from c86c90e to dfa04ff Compare September 6, 2024 18:06
@c4rlosviteri c4rlosviteri force-pushed the pr/update-to-sd-v4 branch 2 times, most recently from 0674c47 to ebc3d8b Compare September 12, 2024 15:11
@sikhote sikhote removed the request for review from caseycarroll September 20, 2024 20:58
@sikhote sikhote merged commit 47749f5 into next Sep 20, 2024
1 check passed
@sikhote sikhote deleted the pr/update-to-sd-v4 branch September 24, 2024 17:48
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.

3 participants