-
Notifications
You must be signed in to change notification settings - Fork 50
[WIP-02] [Project Solar / Phase 1 / Tokens Pipeline] Add $modes
support to tokens pipeline
#3239
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
base: project-solar/phase-1/HDS-5216_modes/modes-feature-branch
Are you sure you want to change the base?
Conversation
The latest updates on your projects. Learn more about Vercel for GitHub.
|
$modes
support to tokens pipeline$modes
support to tokens pipeline
bcb7219
to
fa4bad2
Compare
1803ce4
to
66b876e
Compare
66b876e
to
1a855c1
Compare
1a855c1
to
57ea7fa
Compare
$modes
support to tokens pipeline$modes
support to tokens pipeline
f4d5ff3
to
ce5300d
Compare
ce5300d
to
ba2e78d
Compare
ba2e78d
to
b81cb39
Compare
b81cb39
to
aa74131
Compare
aa74131
to
723b081
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.
These updates to the 2 JSON files seem to make sense. They are just adding the "mode" or "theme" values for each type of color token, correct? That seems fairly straight forward in and of itself to me.
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.
My first question would be if there's perhaps a better/clearer name for this file than "generateExtraThemingFiles.ts"?
Any other options you considered? The word "extra" seems a bit ambiguous.
outputContent = `${header}\n\n`; | ||
// | ||
// these are the themed `carbonized` tokens | ||
outputContent += `@media (prefers-color-scheme: light) { ${cds0ThemedSource} }\n\n`; |
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.
https://drafts.csswg.org/mediaqueries-5/#prefers-color-scheme
In the W3C specs for the prefers-color-scheme
media query, it mentions that a value of "light" indicates EITHER that the user expressly set a preference for a light color scheme OR that they did not set a preference and thus the default should be used.
Following this, they suggest using (not (prefers-color-scheme: dark))
vs. (prefers-color-scheme: 'light') to allow for future updates to express a more active preference for a light color scheme or possible other color schemes.
Anyway, it doesn't seem too vital to worry about, but just something I thought we may want to consider.
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.
Did a high level-ish overview of everything, and testing added themes for most of the token types. Will follow this with a deeper review of the scripts, but agree with the overall approach.
|
||
export async function generateExtraThemingFiles(_dictionary: Dictionary, config: PlatformConfig ): Promise<void> { | ||
|
||
const commonSource = await getSourceFromFileWithRootSelector(config, 'hds', 'common-tokens.css'); |
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.
[Issue] Preface: This may not be relevant if all core and semantic / component level tokens are themed.
When a core level token has modes, and a semantic level token that references that token does not have modes, there can be some mismatches in the values output into the css files.
Example: --token-color-blue-200
has modes, but --token-color-foreground-action
, which is set to --token-color-blue-200
, does not.
"blue-200": {
"$type": "color",
"$value": "#1060ff",
"group": "palette",
"$modes": {
"hds": "#1060ff",
"cds-g0": "{carbon.color.blue.20}",
"cds-g10": "{carbon.color.blue.20}",
"cds-g90": "{carbon.color.blue.20}",
"cds-g100": "{carbon.color.blue.20}"
}
},
with-root-selector
In this situation, for the with-root-selector
files, --token-color-blue-200
is correctly added to themed-tokens.css
with it's correct value.--token-color-foreground-action
is still added to common-tokens.css
, but it's value is equal to the themed value for --token-color-blue-200
because of the token aliasing.
This leads to a situation where a variable could have different values across the common-tokens.css
files depending on the value of the themed core token it references.
hds/common-tokens.css
--token-color-foreground-action: #1060ff;
cds-g10/common-tokens.css
--token-color-foreground-action: #d0e2ff;
Extra theming files
For the extra theming files created here using commonSource
, only the common-tokens.css
from the hds
theme is used. This leads to a situation where the semantic tokens is always set to the original blue 200 core token value, not a themed one.
This all raises a question of if a core level token is themed is the expectation that the semantic level one should always reflect the themed value of the core tokens, or only the HDS value of the core token?
However, if all of the semantic tokens have their own theming set then this is no longer an issue. So if that is the plan then this is a moot point.
"-apple-system", | ||
"BlinkMacSystemFont" | ||
], | ||
"$modes": { |
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.
[Note] Overall think this approach is clean and I agree with it.
My one thought was there could be many cases where all of the carbon tokens are equal for every carbon theme, essentially Carbon's "common tokens". With this approach there could be a lot of duplication of values for tokens across the carbon themes. One alternative could be to have a nesting of the themes (Carbon, HDS), and then modes within those themes (g0, g10, etc.).
Ultimately I think the complexity of that would outweigh the benefits, as HDS only has one theme anyway and there could be plenty of instances of carbon variables differing across themes depending on how the variable mapping goes. This approach is the most flexible to accommodate those kinds of things.
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.
[Note] When testing adding $modes
to font-size
tokens it was successful, but there are some small things to note when theming them.
Our font size tokens are set using px, while Carbon's are set using rems. Both tokens have a unit
field which specifies the unit of the value provided.
When the tokens are generated both the px values and rem values are successfully converted to rems. However there are two small places where that conversion does not happen as accurately.
- In the comment appended to the end of the CSS token, the values listed are the original HDS token values not the themed ones.
- In the token docs json the hds mode will be missing its unit
"$modes": {
"hds": "30",
"cds-g0": "2.625rem",
"cds-g10": "2.625rem",
"cds-g90": "2.625rem",
"cds-g100": "2.625rem"
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.
[Note] I did a test of adding $modes
to most of the data types in the token library. Below are my preliminary results.
- Color
- Hex codes
- Alpha values
- Typography
- font family
- font weight
- font size
- Mostly correct - see comments
- line height
- Token comments incorrect
- letter spacing
- Border radius
- Carbon has no border radius tokens, but setting them to a hard coded value worked
- Animation easings
- Differences in the json format across HDS and Carbon
- Background image / icons
- Will probably need a custom solution to get carbon icons into the background-image tokens.
📌 Summary
This is the implementation PR for adding support to co-located
$modes
values in the design tokens JSON files, the processing of such values, and the generation of distinct "themed" CSS files in output, with different formats.This is built on top of #3238
🛠️ Detailed description
In this PR I have:
build.ts
scriptgetStyleDictionaryConfig.ts
file, so there's less noise in the main file, and it's much easier to see what the configurations are and how they're set upreplace-value-for-mode-${mode}
that tricks Style Dictionary (SD) by replacing all the expected$value
entries/values with the corresponding$modes[mode]
values, before the tokens are processed by SDattributes/themeable
that adds athemeable: true
entry to a token object, if the token itself has a$modes
key, or one of its alias references/ancestors has a$modes
key"{foo.bar.baz} {xxx.yyy.zzz}"
) it's not working; this will be tackled in a separate PR, since it's a very specific code change to make, and it can be done in isolationgenerate-extra-theming-files
that executes the code of thegenerateExtraThemingFiles.ts
which post-processes the single-theme CSS files generated by SD (underthemed-tokens/with-root-selector
) and combines them in different formatswith-prefers-color-scheme
for consumers (eg Boundary) that use only@media(prefers-color-scheme)
to control the theme of their applicationg0
andg100
are availablewith-css-selectors
for consumers that will use the HDS-provided theme switcher or follow the HDS guidance of how to implement theming by themselves (this uses[data-hds-theme=***]
and.hds-theme-***
as CSS selectorsg0
,g10
,g90
,g100
are available (in the future this may change, and we may want to expose onlyg0
andg100
also for this approachwith-combined-strategies
is a mix of the previous two, where consumers may want to give their end-user the option to choose betweenlight
,dark
, andauto
(system settings via prefers-color-scheme)g0
,g10
,g90
,g100
are available (in the future this may change, and we may want to expose onlyg0
andg100
also for this approachwith-scss-mixins
is unlikely to be used, but it was super east to implement, and it provides a simple way for consumers to include theming in their codebase in an extremely customized wayFor testing purposes I have also:
color.palette.neutral-0
,color.foreground.primary
, and a few typography tokens) adding an extra$modes
block of values (some of them, referencing the Carbon tokens extracted by our pipeline from the@carbon/***
packages)🗒️ TODOs
Things that still need to be done before merging
Things that still need to be done (in fast-follow-up PRs)
outputReferences
logic and thethemeable
transformoutputReferences
function (in which case, understand which references should be outputted and which shouldn't) or we keep it tofalse
attributes/themeable
transformation non working when the alias is composed🔎 How to review
$modes
are defined in thesrc
JSON files, and see if that makes sense to you as future code maintainergetStyleDictionaryConfig
and see if the code as is makes sense to you, as possible future maintainer, if that's enough understandable on what it does, on how things are named, etcgenerateExtraThemingFiles
and see if the code as is makes sense to you, as possible future maintainer, if that's enough understandable on what it does, on how things are named, etc:where()
to reduce the specificity, something I've seen being done somewhere else with theming files)🛠️ How to test
Add more
$modes
entries to thesrc
JSON tokens (taking inspiration by the existing ones) and then run thepnpm build
command, and see how the different values are reflected in the generated CSS token files. Try to test different combinations of aliasing, edge cases you can think of, etc. In particular check that the different theme values are reflected in the corresponding theme files, under the correct CSS selectors, and that the themed and non-themed tokens are correctly split in common and themed files. If you add only$modes
values, the "old" tokens should remain unaltered.🔗 External links
Jira tickets
👀 Component checklist
A changelog entry was added via Changesets if needed (see templates here)💬 Please consider using conventional comments when reviewing this PR.
📋 PCI review checklist
Examples of changes to controls include access controls, encryption, logging, etc.
Examples include changes to operating systems, ports, protocols, services, cryptography-related components, PII processing code, etc.