RFC: Semantic shared colors #27679

miroslavstastny · 2023-04-24T23:08:25Z

All color palette tokens in the theme are named after colors. This RFC discusses options how we could name some of them using semantic names (danger, success...).

Related issues: #26168, #25720.

PREVIEW

fabricteam · 2023-04-24T23:20:30Z

📊 Bundle size report

_{🤖 This report was generated against 8ec648d4060906a32bad46a16e0eae13a7aa5959}

codesandbox-ci · 2023-04-24T23:25:56Z

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 27b7d35:

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

size-auditor · 2023-04-24T23:26:46Z

Asset size changes

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

Baseline commit: 8ec648d4060906a32bad46a16e0eae13a7aa5959 (build)

behowell · 2023-04-26T17:13:44Z

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

+### 4. Add semantic color tokens mapped to the current shared colors
+
+All tokens are mapped to CSS variables with the same name: `colorPaletteRedBackground1` token is mapped to `--colorPaletteRedBackground1` CSS variable. From the very first release of FUIR9, we [always warned partners](https://react.fluentui.dev/?path=/docs/concepts-developer-theming--page#do-not-use-css-variables-directly) that they should never depend on this fact:
+
+> Do not use CSS variables directly
+>
+> ⚠ Never use theme CSS variables directly! The CSS variables implementation of the theme is internal to the library. We might eventually decide to change the variable names, hash them or even use direct values instead of some variables. Always use the tokens to access the theme.
+
+Add "status" slots to `cranberry`, add `orange` as a status color.
+
+Add semantic color tokens which are mapped to the current shared colors. For example, `colorPaletteDangerBackground1` is mapped to `--colorPaletteRedBackground1`. `Danger` is still mapped to the old `red` color, but the token name is now semantic. Still not changed to `cranberry`.
+
+Update the sources in the library to use the new colors.
+
+Update the mapping from old colors to the new colors.


Summarizing a suggestion made offline:
How about instead using the new token names ("danger", etc.) as css variables, and mapping the old (deprecated) token names ("red", etc.) to the new css variables:

/** @deprecated Use colorPaletteDangerBackground2 or colorPersonaRedBackground2 */ tokens.colorPaletteRedBackground2 = 'var(--colorPaletteDangerBackground2)'

This should mitigate/solve the first two cons of option 4:

👍 It's possible to override danger separately from persona-red.

👍 The css variable names map to the new, recommended token names for easier debugging.

One additional con of this:

👎 If anyone is using the css variable names directly, they will break when we rename them to the new names.

👍 The css variable names map to the new, recommended token names for easier debugging.

This makes sense, we should optimize debugging for the recommended use case.

My question would be if there would be a future for colorPaletteRed as a token. If the intention is for users to stop using it and only use colorPaletteDanger (i.e. remove this token on next major), then @behowell's suggestion makes sense and I would vote for that.

If we still expect both tokens to be used past the next major, I don't think any kind of mappings in the FUI theme will be scalable because it overriding one token can affect other tokens. This would apply to any mapped tokens we intend to introduce.

I'm in favour of both @behowell idea and 1B: "deprecate" the tokens and create a V2 theme which does not contain them - let applications switch when they are ready.

that v2 gradual approach is something we will be adopting in "near-ish future" so would nicely align. also it would mitigate "unexpected" breaking changes.

Added as option 4B.

It makes sense for colors which we are 'deprecating' - green and yellow should not be used anywhere so we introduce success and for backwards compatibility we map green to the new success.

👍 It's possible to override danger separately from persona-red.

Huh, actually I had ignored the fact that in option 4 we would have a different shape of theme and tokens. Added to Open Issues, will look into it next week.

that v2 gradual approach is something we will be adopting in "near-ish future" so would nicely align.

Realized the same problem here - introducing v2 theme would also require introducing v2 tokens which would require an import change in all .styles.ts files in the partner codebase :/ What would be our solution? Codemod?

My question would be if there would be a future for colorPaletteRed as a token. If the intention is for users to stop using it and only use colorPaletteDanger (i.e. remove this token on next major), then @behowell's suggestion makes sense and I would vote for that.

This is why I would rather prefer 4 to 4B out of these two options - red is still valid.
And I can understand danger being mapped to red (because the color is red). But I do not understand red being mapped to danger (but I might be biased).

Also designs change - as we see, danger mapping is changing from red to cranberry - semantic colors are less stable. But whatever is red will always be red - I would expect less changes here.

behowell · 2023-04-26T17:14:47Z

I'll review this on behalf of @microsoft/cxe-red.

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

miroslavstastny · 2023-04-26T22:42:14Z

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

+| `lightGreen` | available    | available | No change                                    |
+| `marigold`   | away         | away      | No change                                    |
+
+_‼️ TODO: verify with design_


NOTE: after discussing the topic with @daisygeng, the current Design requirement is to introduce semantic color names only for danger, success and warning. I will be updating the RFC accordingly.

Updated the spec accordingly - if we ignore presence tokens, introducing just status tokens now shows different numbers (of tokens eventually added/affected).

tudorpopams · 2023-04-27T12:14:17Z

@ValentinaKozlova will review this on behalf of @microsoft/cxe-prg

mltejera · 2023-04-30T21:37:07Z

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

+- 👎 We will remove some "basic" colors `green`, `yellow` - are we sure those have been used as semantic colors only?
+  approach
+- 👎 1A: adds ~30 new tokens
+- 👎 If design decides to do the same for **presence** tokens, we will need to add another ~40 tokens to follow the same


And I'm sure there will be new families of tokens requested for scenarios we don't forsee today.

ValentinaKozlova · 2023-05-02T16:46:29Z

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

+| `darkOrange` | severe                | **severe** should not be used anywhere. Keep `darkOrange` (temporarily) for Teams compatibility.                                    |
+| `yellow`     | warning               | Replace by semantic `warning`, mapping changed from `yellow` to `orange`                                                            |
+| `berry`      | OOF                   | No change                                                                                                                           |
+| `lightGreen` | available             | No change                                                                                                                           |


It doesn't look consistent that some tokens are named after colors e.g. lightGreen and some are after statuses, e.g. success. Maybe in future all tokens will use semantic names but what if these tokens will be needed not only for statuses

Agree here. Let's take the current usages of red as an example (and for now ignore the fact that we are changing the danger from red to cranberry).

red is used for:

danger

busy

persona

Now we will introduce danger status semantic color. But we cannot deprecate the red because that is still used for busy and persona.

Eventually we might decide to introduce presence semantic colors and add busy. After that we still need the red for persona so we cannot remove it, but can deprecate the slots used only for presence.

Also, this covers only the usages in the core library, but not partners' codebases. Design provides partners with guidelines but we cannot be 100% sure about what they really do.

layershifter · 2023-05-04T14:34:38Z

I will review it on behalf of @microsoft/teams-prg

layershifter · 2023-05-05T09:51:47Z

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

+
+## Summary
+
+All color palette tokens in the theme are named after colors. This RFC discusses options how we could name some of them using semantic names (`danger`, `success`...).


"All color palette tokens in the theme are named after colors."

Not sure that understand WDYM there.

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

layershifter · 2023-05-05T09:55:10Z

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

+| `colorPaletteRedBorder1`            | status                |
+| `colorPaletteRedBorder2`            | status                |
+
+This results in 150 (28 _ 3 + 7 _ 9 + 3) color tokens.


What means "_" there? Should it be "*"?

Stubborn Prettier 🤷‍♂️

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

behowell · 2023-05-08T23:48:47Z

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

+- 👎 There might be code which depends on the 1:1 token to CSS variable mapping (hello, [`themeToTokensObject`](https://github.com/microsoft/fluentui/blob/97c1c1ab218afb20e37bfc35936fd88177d439b4/packages/tokens/src/themeToTokensObject.ts#L14-L16)).
+- 👍 4B: The css variable names map to the new, recommended token names for easier debugging.
+- 👎 4B: If anyone is using the css variable names directly, they will break when we rename them to the new names.
+- ‼️ In the current implementation we use `Theme` type for theme definition and `Record<keyof Theme, string>` for tokens in styles. In option 4 this would have to change - see [Open Issues](#open-issues).


I'm not sure I understand the Record<keyof Theme, string> issue here, but I also haven't spent much time looking at the theme code, so I might be misunderstanding the problem.

The way I was thinking of option 4B: the Theme object would contain entries for ALL of the old and new tokens, as if we had just gone and added a bunch of new tokens for the new color names, while keeping the old deprecated ones around. However, internally the deprecated tokens would map to the new CSS variable names to avoid significantly increasing our css variable count. But the fact that we're reusing the same CSS variable names wouldn't be exposed in the public API.

There are two different objects - theme and tokens:

theme is a "list of css variables" and their values which will be added to DOM.

tokens are used in styles.ts files and map from Javascript object properties to css variable strings

Up until now, for every property which exists in tokens object, there is property in theme object and vice versa. This is enforced by the Theme typings.

In options 4, we are adding a new item to tokens, but map it to an existing item in theme.

behowell · 2023-05-08T23:49:20Z

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

+- K: `colorPalette` (`colorPaletteDangerBackground1`)
+- L: `colorStatus` (`colorStatusDangerBackground1`)
+- M: `colorSemantic` (`colorSemanticDangerBackground1`)
+- n: anything else?
+
+Also consider how the proposed name would work if we decide to introduce semantic **presence** tokens (`away`).


I like colorStatus. If we add presence colors, we could use colorStatus for that as well, or colorPresence if we wanted to distinguish them.

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

micahgodbolt · 2023-05-17T22:09:05Z

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

+- 👎 1A: can cause temporarily inconsistent UI colors between library and partners
+- 👍 1B: we can use the current mapping in v1 theme and the new mapping in v2 theme - partners can control the moment when they switch to the new mapping
+- 👎 1B: requires us maintaining the double amount of themes
+- 👎 1C: breaks partners (the questions is how much, but...)


can geoff's query tool track down uses of those tokens?

I checked just TMP - there are 12 usages of green, 5 usages of yellow, but 121 usages of red.
So 1C would be difficult.

But even without breaking changes, we need to be careful with switching from red to cranberry regardless the name (danger or not) - some usages mean danger, other usages might mean persona or status - partners need to review all the usages and decide what the intent is and change accordingly.
To avoid inconsistent UI, best way forward would be to introduce danger and map it to red, let partners update their usages and only after that change danger from red to cranberry.

micahgodbolt · 2023-05-17T22:11:30Z

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

+
+- 👍 Change does not affect components or partner code.
+- 👍 No new tokens.
+- 👎 Designers will scream as "Yellow and orange are two different colors!".


at least they would learn to stop naming color tokens by the color names?

miroslavstastny · 2023-05-24T14:58:32Z

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

+- 👍 Semantic tokens without any unnecessary tokens.
+- 👍 Controlled partner upgrade from old hex values to new values, but coupled with library upgrade.
+- 👎 This is currently not supported by token pipeline - would require either changes there or custom implementation on library side.
+- 👎 Semantic and status tokens are still coupled - the only way for the application to override `danger` is to override `red` (which will affect other parts of the UI).


This (the impossibility to override) might confuse partners, as they will want to override the danger, it will just not work.

miroslavstastny · 2023-07-24T21:35:14Z

After considering all pros and cons and discussing the requirements with design again, I decided to add new status tokens for danger, success and warning, named colorStatus..., for example colorStatusDangerBackground1.
This approach adds 30 tokens, does not bring any new concept and allows existing and new tokens to be independently overriden.
See the implementation in #28006.

miroslavstastny · 2024-07-08T19:07:12Z

This was implemented a year ago by just adding the required tokens. Closing the issue.

miroslavstastny requested a review from a team as a code owner April 24, 2023 23:08

msft-fluent-ui-bot assigned ling1726 Apr 24, 2023

github-actions bot added this to the April Project Cycle Q2 2023 milestone Apr 24, 2023

github-actions bot added the Type: RFC Request for Feedback label Apr 24, 2023

rfc: Semantic shared colors

9666d9e

miroslavstastny force-pushed the rfc/semantic-shared-colors branch from cf546ce to 9666d9e Compare April 24, 2023 23:14

miroslavstastny assigned miroslavstastny and unassigned ling1726 Apr 24, 2023

miroslavstastny requested review from a team April 24, 2023 23:17

miroslavstastny changed the title ~~rfc: Semantic shared colors~~ RFC: Semantic shared colors Apr 24, 2023

marcosmoura self-requested a review April 25, 2023 12:11

behowell reviewed Apr 26, 2023

View reviewed changes

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

miroslavstastny commented Apr 26, 2023

View reviewed changes

tudorpopams requested review from ValentinaKozlova and removed request for marcosmoura April 27, 2023 12:13

miroslavstastny requested a review from brianchristopherbrady April 28, 2023 20:44

Incorporate design feedback

5276e6b

mltejera reviewed Apr 30, 2023

View reviewed changes

ValentinaKozlova reviewed May 2, 2023

View reviewed changes

layershifter reviewed May 5, 2023

View reviewed changes

ValentinaKozlova reviewed May 5, 2023

View reviewed changes

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

behowell reviewed May 8, 2023

View reviewed changes

PR comments

6039320

micahgodbolt reviewed May 17, 2023

View reviewed changes

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

micahgodbolt reviewed May 17, 2023

View reviewed changes

github-actions bot modified the milestones: April Project Cycle Q2 2023, May Project Cycle Q2 2023 May 24, 2023

miroslavstastny added 2 commits May 23, 2023 21:41

address PR comments, add code samples

10a1b0e

added token pipeline con to option 4

27b7d35

miroslavstastny commented May 24, 2023

View reviewed changes

miroslavstastny closed this Jul 8, 2024


		## Summary

		All color palette tokens in the theme are named after colors. This RFC discusses options how we could name some of them using semantic names (`danger`, `success`...).

RFC: Semantic shared colors #27679

RFC: Semantic shared colors #27679

Uh oh!

Conversation

miroslavstastny commented Apr 24, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fabricteam commented Apr 24, 2023 • 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 Apr 24, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

size-auditor bot commented Apr 24, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Asset size changes

Uh oh!

behowell Apr 26, 2023 • 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!

ling1726 Apr 27, 2023 • 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!

Choose a reason for hiding this comment

Uh oh!

behowell commented Apr 26, 2023

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tudorpopams commented Apr 27, 2023

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!

layershifter commented May 4, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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!

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!

miroslavstastny May 24, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

miroslavstastny commented Apr 24, 2023 •

edited

Loading

fabricteam commented Apr 24, 2023 •

edited

Loading

codesandbox-ci bot commented Apr 24, 2023 •

edited

Loading

size-auditor bot commented Apr 24, 2023 •

edited

Loading

behowell Apr 26, 2023 •

edited

Loading

ling1726 Apr 27, 2023 •

edited

Loading

miroslavstastny May 24, 2023 •

edited

Loading