[web-components]: remove design token implementation in favor of platform solution #30002
Conversation
📊 Bundle size report🤖 This report was generated against 12da47698c9121c0345c2f0db481cbae7a91b66e |
| @@ -0,0 +1,7 @@ | |||
| { | |||
There was a problem hiding this comment.
🕵 fluentui-web-components-v3 Open the Visual Regressions report to inspect the affected screenshots
Slider 2 screenshots
| Image Name | Diff(in Pixels) | Image Type |
|---|---|---|
| Slider.Slider Steps Vertical.normal.chromium.png | 1 | Changed |
| Slider.Slider Steps.normal.chromium.png | 1 | Changed |
There was a problem hiding this comment.
This looks okay to me but I let a WC Codeowner mark this as resolved after reviewing.
Asset size changesSize Auditor did not detect a change in bundle size for any component! Baseline commit: 12da47698c9121c0345c2f0db481cbae7a91b66e (build) |
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit 07cd8a4:
|
| @@ -10,12 +9,12 @@ const tokenNames = Object.keys(tokens) as (keyof Theme)[]; | |||
| */ | |||
| export const setTheme = (theme: Theme) => { | |||
There was a problem hiding this comment.
The great thing about this now, is that we don't need to use this function at all if we can set the tokens we care about in the HTML document directly, since everything uses platform. Thank you!
There was a problem hiding this comment.
Was that needed previously?
| color: var(${tokens.colorNeutralForegroundOnBrand}); | ||
| border: var(${tokens.strokeWidthThicker}) solid var(${tokens.colorNeutralStroke1}); | ||
| padding: var(${tokens.spacingVerticalS}) var(${tokens.spacingHorizontalM}); | ||
| box-shadow: var(${tokens.shadow28}); |
There was a problem hiding this comment.
Did you mean to grab named exports here?
| export const shadow8Brand = '--shadow8Brand'; | ||
| export const shadow16Brand = '--shadow16Brand'; | ||
| export const shadow28Brand = '--shadow28Brand'; | ||
| export const shadow64Brand = '--shadow64Brand'; |
There was a problem hiding this comment.
Let's discuss taking a common dep on @fluentui/tokens. We're nearly identical between this and that now:
https://github.com/microsoft/fluentui/blob/master/packages/tokens/src/tokens.ts
Obviously named exports over objects is a thing. That can be handled many ways to ensure minimal bundles.
There was a problem hiding this comment.
Yeah, I'm fine with generating this locally at build time for instance - I didn't want to block the improvement on that.
There was a problem hiding this comment.
At least there should be a test to verify the tokens here match the tokens in @fluentui/tokens
chrisdholt
force-pushed
the
users/chhol/remove-design-token-in-favor-of-platform-solution
branch
from
b16b34d
to
e2a32fa
Compare
|
@chrisdholt what is remaining for this to land, do you need any help? |
chrisdholt
merged commit 4909fe2
into
microsoft:web-components-v3
chrisdholt
deleted the
users/chhol/remove-design-token-in-favor-of-platform-solution
branch
Previous Behavior
Previous implementation (WC v2) leveraged the FAST Design token implementation which is helpful when you need to observe values which respond to one another.
New Behavior
With the shift to Fluent 2 tokens which are static and do not need to relate to one another, the implementation can be simplified. This PR removes the design token implementation in favor of platform CSS variables. The current proposal takes into account other long term goals of supporting things like tree shaking tokens, fallback/default values and or tokens, which is why the token names are strings and the variables are set within the components.
This is an initial example of what this could look like - open to feedback.