-
Notifications
You must be signed in to change notification settings - Fork 77
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!: refactor design tokens and token transformer #8149
Conversation
c2f17f2
to
34d4a6e
Compare
@alisonailea Awesome! Some pre-review notes:
|
It also looks like this is addressing #8141. If so, can this be rolled back and tackled separately to have more focused PRs? If not, can you comment on this and cover it in the breaking changes notes? |
This must be singular to adhere to the spec. |
set old core tokens for deprecation update token types align token reference syntax with design token standards
BREAKING CHANGE: removes “light” and “dark” tokens in favor of composite color mode tokens
BREAKING CHANGE: new platform output files. Replaces “headless” with “global” and removes “calcite” from filenames. Significantly reduces the filesize of platform output files. Now supports composite color mode tokens and typography tokens Adds multi-file output, including ./index files
removes unused $metadata
34d4a6e
to
5b10c62
Compare
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 is looking great, @alisonailea!
Regarding the spell checker, I think we can use a very terse explainer for ignored lines we can reuse for now. If we can separate files extended from sd-transforms
, we can ignore those entirely.
}, | ||
"23": { | ||
"value": "144px", | ||
"type": "sizing" | ||
"type": "sizing", | ||
"description": "For backwards compatibility only. This token will be removed from the published tokens in the next major version because the type \"size\" is prefered by designer", |
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.
Great job on adding a deprecation notice for these. ✨⭐✨
}, | ||
"24": { | ||
"value": "160px", | ||
"type": "sizing" | ||
"type": "sizing", | ||
"description": "For backwards compatibility only. This token will be removed from the published tokens in the next major version because the type \"size\" is prefered by designer", |
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.
Typo: prefered
➡️ preferred
.
Question: should designer
be pluralized?
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.
thanks for the catch. Should be "preferred by design"
"border-radius": { | ||
"value": "{core.border.border-radius.none}", | ||
"type": "borderRadius", | ||
"description": "For backwards compatibility only. This token will be removed from the published tokens in the next major version in favor of removing the \"ui\" level that doesn't match the naming schema.", |
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 think this needs to be updated according to the latest breaking change notes in the description:
- exported token no longer include "app" or "ui"
Applies to other items with the same description.
"light": { | ||
"value": "{core.font.font-weight.light}", | ||
"type": "fontWeights", | ||
"description": "only for Avenir Next World (secondary font family)" |
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.
Nitpick: For consistency, this description should also start capitalized.
"7": { | ||
"value": "{core.font.font-size.7}", | ||
"type": "fontSizes", | ||
"description": "For backwards compatibility only. This token will be removed from the published tokens in the next major version in favor of t-shirt sizing tokens.", |
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.
t-shirt sizing
TIL 🌠🤩
|
||
export function formatTokens(dictionary: Dictionary, args: MappedFormatterArguments): FormattedTokens { | ||
const allTokens = [...dictionary.allTokens].sort(sortByReference(dictionary)); | ||
// const formatting = FormattingRules[args.options.platform] || FormattingRules.default; |
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.
Please remove all commented-out code.
@@ -0,0 +1,52 @@ | |||
/** | |||
* This is a copy/extension of sd-transforms/src/parsers/resolveReference.ts but now with better types! |
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.
Can you make sure all copies have a note on them? I think we need a way to separate them.
How will we keep these in sync with the upstream source?
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.
We don't have to. I pulled in the fuctions they use that I think are helpful but since design is moving away from Token Studio we should not need to keep track of the upstream source.
import { Options } from "../../../types/styleDictionary/options.js"; | ||
import { PossibleRegistryArgs } from "../utils.js"; | ||
import { PlatformOptions } from "../../../types/styleDictionary/platform.js"; | ||
// import { TransformedToken } from "../../../types/styleDictionary/transformedToken.js"; |
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.
Please remove commented-out code.
tokens: TransformedTokens; | ||
allProperties: TransformedToken[]; | ||
properties: TransformedTokens; | ||
usesReference: (value: any) => boolean; |
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.
Can the type be narrowed or is it unknown?
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 could potentially be anything at this point.
"value": "$semantic.ui.color.foreground.1.dark", | ||
"type": "color" | ||
} | ||
"value": "{semantic.ui.color.foreground.1}", |
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 noticed this was the only component-level token that has a description. Is it intentional? If so, will others be updated as well?
On a similar note, this was the only one I found where background
entry that didn't have a default
nor light/dark
keys right under it.
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.
component tokens haven't been fully updated yet
@alisonailea Thanks for updating the PR title/description, btw! |
@alisonailea, we had to recreate
Sorry for the trouble! 🙇 Recreated PR: #8215 |
Related Issue: #7325 #8141
Summary
This is a complete refactor of the token transformer. This introduces breaking changes to tokens.
BREAKING CHANGE:
--calcite-app-breakpoint-width-lg-min
is now--calcite-container-size-width-lg-min
)border-border-width
is nowborder-width