RFC: Reduce number of shared color alias tokens #23537

miroslavstastny · 2022-06-14T13:58:38Z

This RFC proposes to reduce number of shared color alias tokens to improve performance.
Changing the tokens would be a BREAKING CHANGE!

PREVIEW

fabricteam · 2022-06-14T14:05:34Z

📊 Bundle size report

_{🤖 This report was generated against d4db5211e7a53eaa7910b87f737bd959218a78e0}

codesandbox-ci · 2022-06-14T14:05:53Z

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 133c76a:

Sandbox	Source
@fluentui/react 8 starter	Configuration
@fluentui/react-components 9 starter	Configuration

size-auditor · 2022-06-14T14:12:29Z

Asset size changes

Size Auditor did not detect a change in bundle size for any component!

Baseline commit: d4db5211e7a53eaa7910b87f737bd959218a78e0 (build)

spmonahan · 2022-06-14T16:28:11Z

rfcs/react-components/convergence/theme-shared-colors.md

+- 👍 Reducing shared color alias tokens from 441 to ~225 instead of increasing them to ~600 (to add tokens needed for Calendar)
+- 👍 More clarity around which specific color to use for semantic states instead of relying on design guidance
+- 👍 Allows a bit more wiggle room for the semantic alias table to grow w/o adding too much bloat
+- 👎 **Breaking change after the final RC**


For me this is the reason to not make this change, or rather, to make it in a non-breaking way.

As you note below this should not affect partners much but my experience with other breaking changes is that they can require quite a bit of coordination between partners. Historically we've reserved the right to make breaking changes to resolve accessibility issues and my belief is that performance is an accessibility issue but we should still strive to make these types of changes in a non-breaking way whenever possible.

While it is not ideal to ship a first release with deprecated features (in fact this was a motivation for breaking changes we've previously made during RC) my feeling is that we're getting too close to the stable release to ship a breaking change that isn't resolving a hard blocking issue. I don't feel this is a hard blocking issue for release as we can ship "v2" or "perf" versions of all themes and update our docs to demonstrate their use in all cases.

I do think this is a good change to make and I'm not strictly opposed to making it in a breaking way, I would just prefer to do so in a backwards compatible way given our release schedule.

I tend to agree with @spmonahan here, we should do this either in a non-breaking way or change our release schedule.

Can we please prioritize actually asking partners about this? they stand the most to gain/lose for perf/breaking change in this scenario

I do think this is a good change to make and I'm not strictly opposed to making it in a breaking way, I would just prefer to do so in a backwards compatible way given our release schedule.

we agreed that BC in RC phase can be done if really necessary, so landing this prior to stable should be ok.

Also dunno if you folks read reasoning regarding why v2 was discarded, but it gives you good additional context

@spmonahan has been working most closely with partners on BC, so he should have a sense of their tolerance. That being said, I agree with @ling1726 that flighting this with our core partners is a the right call.

let me put on my partner hat. v9 adoption in Teams is currently blocked by the perf issue - injecting too many tokens delays initial render and degreades some of the perf metrics. I don't know if all partners track this metrics but probably the initial render is critical for most of them. In production rings this is up to 300ms regression in average and we have seen the cost being linear with relation to the amount of tokens.

Migration to new theme might have some cost for the partners and it will have cost for Teams, but as far as I undrstand it, it will be a change that breaks build and is easily detectable.

If we decide to deprecate the old theme, partners will need to migrate to the new theme anyway.

Since we are in RC, I think we should not release final version knowing that a change like this is planned. We should make the change and explain the benefits.

rfcs/react-components/convergence/theme-shared-colors.md

khmakoto · 2022-06-14T19:14:48Z

rfcs/react-components/convergence/theme-shared-colors.md

+- 👍 Reducing shared color alias tokens from 441 to ~225 instead of increasing them to ~600 (to add tokens needed for Calendar)
+- 👍 More clarity around which specific color to use for semantic states instead of relying on design guidance
+- 👍 Allows a bit more wiggle room for the semantic alias table to grow w/o adding too much bloat
+- 👎 **Breaking change after the final RC**


I tend to agree with @spmonahan here, we should do this either in a non-breaking way or change our release schedule.

Hotell

approving in terms of technological solution to our design system, lets get alignment regarding the approach - BC / v2

Hotell · 2022-06-15T10:16:27Z

rfcs/react-components/convergence/theme-shared-colors.md

+
+The theme currently contains 49 global shared colors.
+
+Current alias structure forces each ramp to be uniform (the same set of alias tokens for each shared color), resulting in 49 alias tokens each time we need a new line item (new alias token). Today we have 9 alias slots = total alias count is 441 (49x9).


q: can you pls elaborate what ramp means?

the same set of alias tokens for each shared color :-) I will try to describe it better.

rfcs/react-components/convergence/theme-shared-colors.md

Hotell · 2022-06-15T10:24:51Z

rfcs/react-components/convergence/theme-shared-colors.md

+- 👍 Reducing shared color alias tokens from 441 to ~225 instead of increasing them to ~600 (to add tokens needed for Calendar)
+- 👍 More clarity around which specific color to use for semantic states instead of relying on design guidance
+- 👍 Allows a bit more wiggle room for the semantic alias table to grow w/o adding too much bloat
+- 👎 **Breaking change after the final RC**


I do think this is a good change to make and I'm not strictly opposed to making it in a breaking way, I would just prefer to do so in a backwards compatible way given our release schedule.

we agreed that BC in RC phase can be done if really necessary, so landing this prior to stable should be ok.

Also dunno if you folks read reasoning regarding why v2 was discarded, but it gives you good additional context

rfcs/react-components/convergence/theme-shared-colors.md

Co-authored-by: Makoto Morimoto <[email protected]> Co-authored-by: Geoff Cox (Microsoft) <[email protected]>

* RFC: Reduce number of shared color alias tokens * Apply suggestions from code review Co-authored-by: Makoto Morimoto <[email protected]> Co-authored-by: Geoff Cox (Microsoft) <[email protected]> * address PR comments Co-authored-by: Makoto Morimoto <[email protected]> Co-authored-by: Geoff Cox (Microsoft) <[email protected]>

RFC: Reduce number of shared color alias tokens

061e199

miroslavstastny added Component: Theme Fluent UI react-components (v9) Type: RFC Request for Feedback labels Jun 14, 2022

miroslavstastny self-assigned this Jun 14, 2022

github-actions bot added this to the June Project Cycle Q2 2022 milestone Jun 14, 2022

miroslavstastny requested review from a team and levithomason June 14, 2022 13:59

spmonahan reviewed Jun 14, 2022

View reviewed changes

khmakoto reviewed Jun 14, 2022

View reviewed changes

Hotell approved these changes Jun 15, 2022

View reviewed changes

JustSlone reviewed Jun 15, 2022

View reviewed changes

rfcs/react-components/convergence/theme-shared-colors.md Show resolved Hide resolved

NotWoods mentioned this pull request Jun 15, 2022

[Feature]: Expose global colors in @fluentui/react-components #23562

Closed

1 task

miroslavstastny mentioned this pull request Jun 16, 2022

BREAKING: reduce shared colors #23578

Closed

GeoffCoxMSFT reviewed Jun 16, 2022

View reviewed changes

rfcs/react-components/convergence/theme-shared-colors.md Show resolved Hide resolved

ling1726 approved these changes Jun 17, 2022

View reviewed changes

spmonahan approved these changes Jun 17, 2022

View reviewed changes

miroslavstastny mentioned this pull request Jun 20, 2022

BREAKING: reduce shared colors #23608

Merged

miroslavstastny and others added 2 commits June 27, 2022 13:08

Apply suggestions from code review

c087e47

Co-authored-by: Makoto Morimoto <[email protected]> Co-authored-by: Geoff Cox (Microsoft) <[email protected]>

address PR comments

133c76a

miroslavstastny merged commit d5d510b into microsoft:master Jun 27, 2022

miroslavstastny deleted the rfc/reduce-shared-colors branch June 27, 2022 11:36


		The theme currently contains 49 global shared colors.

		Current alias structure forces each ramp to be uniform (the same set of alias tokens for each shared color), resulting in 49 alias tokens each time we need a new line item (new alias token). Today we have 9 alias slots = total alias count is 441 (49x9).

RFC: Reduce number of shared color alias tokens #23537

RFC: Reduce number of shared color alias tokens #23537

Uh oh!

Conversation

miroslavstastny commented Jun 14, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fabricteam commented Jun 14, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

📊 Bundle size report

Uh oh!

codesandbox-ci bot commented Jun 14, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

size-auditor bot commented Jun 14, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Asset size changes

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Hotell Jun 15, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Hotell left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Hotell Jun 15, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants

miroslavstastny commented Jun 14, 2022 •

edited

Loading

fabricteam commented Jun 14, 2022 •

edited

Loading

codesandbox-ci bot commented Jun 14, 2022 •

edited

Loading

size-auditor bot commented Jun 14, 2022 •

edited

Loading

Hotell Jun 15, 2022 •

edited

Loading

Hotell Jun 15, 2022 •

edited

Loading