feat(teams-theme): clearing duplicate colors from siteVariables by mnajdova · Pull Request #858 · microsoft/fluent-ui-react

mnajdova · 2019-02-07T11:28:05Z

Description of the problem

In Teams theme we are exporting colors in different ways

through the colors object (containing the natural, emphasis, primitive colors)
directly through siteVariables (we have fields there like orange04, grey02 etc)

This produces big confusion when using the colors. As an example, we have the black color both as siteVariables.black, as well as siteVariables.colors.black. These values can be the same or different (the user doesn't know right away, until they check the values directly in the theme). Moreover, when we look into it, the black color is actually in exported as the siteVariables.colors.grey[900] variant. Why we have black color, if it is the same as some grey variant (it's not even black).

In addition if we look into the values of the colors siteVariables.brand and siteVariables.brand06, they are referencing the same color - siteVariables.colors.primary[500] (why we need two different aliases for the same color?). We also have some colors defined in the siteVariables which are not used at all in the styles and variables (not sure whether they are used on client side)

The aim of this PR is to start reducing the colors defined directly in the siteVariables, as long as we have alias color in the colors object. From here then, we can have clear picture of which colors defined in the siteVariables, do not have mapping in the color palette, and we can iterate on that. Then, we can start creating color palettes for the other themes (dark and high contracts).

Goal

We should aim for clearing all colors directly defined in the siteVariables, and referencing them with something from the colors object.

BREAKING CHANGES (Teams theme)

Some of the colors from siteVariables were removed. Find the mappings below:

brand16 <=> colors.primary[50]
brand14 <=> colors.primary[100]
brand12 <=> colors.primary[200]
brand <=> colors.primary[500]
brand06 <=> colors.primary[500]
brand02 <=> colors.primary[900]

green <=> naturalColors.lightGreen[900]
green04 <=> colors.green[900]

black <=> colors.grey[900]
white <=> colors.grey[50]

red <=> colors.red[900]

yellow <=> colors.yellow[900]

orange04 <=> naturalColors.darkOrange[400]

In addition, the value of the siteVariable.colors.black was changed from siteVariables.colors.grey[900] (#252424) to #000. If the intention of the user was to have the dark grey, then please replace the usage with siteVariables.colors.grey[900].

codecov · 2019-02-07T13:03:59Z

Codecov Report

Merging #858 into master will decrease coverage by <.01%.
The diff coverage is 98.14%.

@@            Coverage Diff             @@
##           master     #858      +/-   ##
==========================================
- Coverage   80.59%   80.58%   -0.01%     
==========================================
  Files         648      649       +1     
  Lines        8334     8320      -14     
  Branches     1420     1420              
==========================================
- Hits         6717     6705      -12     
+ Misses       1602     1600       -2     
  Partials       15       15

Impacted Files	Coverage Δ
...s/teams-dark/components/Tree/treeTitleVariables.ts	`50% <ø> (ø)`	⬆️
...h-contrast/components/Chat/chatMessageVariables.ts	`50% <ø> (ø)`	⬆️
...s/components/RadioGroup/radioGroupItemVariables.ts	`100% <ø> (ø)`	⬆️
...es/teams-dark/components/Header/headerVariables.ts	`50% <ø> (ø)`	⬆️
.../src/themes/teams/components/Menu/menuVariables.ts	`66.66% <ø> (ø)`	⬆️
.../src/themes/teams/components/Text/textVariables.ts	`100% <ø> (ø)`	⬆️
...themes/teams/components/Tree/treeTitleVariables.ts	`50% <ø> (ø)`	⬆️
...emes/teams/components/Chat/chatMessageVariables.ts	`100% <ø> (ø)`	⬆️
...mes/teams/components/Dropdown/dropdownVariables.ts	`100% <ø> (ø)`	⬆️
...es/teams/components/Popup/popupContentVariables.ts	`66.66% <ø> (ø)`	⬆️
... and 14 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f9a6b39...d4caa7d. Read the comment docs.

bmdalex

YAAASSS!!

bmdalex · 2019-02-07T13:38:32Z

packages/react/src/themes/teams/colors.ts

    300: '#8F90C1',
    400: '#6E70AE',
-    500: '#6264A7',
+    500: '#6264A7', // siteVariables.brand, siteVariables.brand06 (same color?)


yes, it's the same color

Thanks for pointing out :D

bmdalex · 2019-02-07T13:48:33Z

packages/react/src/themes/teams-dark/siteVariables.ts

 export const brand06 = '#a6a7dc'
 export const brand08 = '#8b8cc7'
-export const brand12 = brand
+export const brand12 = colors.primary[500]


is there a plan to remove the brand* names from other themes?
that would help A LOT with the color prop story since we won't need any code changes in the styles of the components for other themes (you probably remember the Menu mayhem)

Yep, I remember the problems we had there. In my opinion, ideally we will have colors.primary[500] as a color used for the same thing in all themes, but the themes would change what primary[500] means in that context (it might be lighter in dark theme for example...) Not certain 100% for this yet, we have to have the whole picture first..

This one is actually great example that, in dark theme, colors.primary[500] is used for brand12, although in the light theme it is used as brand. We should rearrange the colors primary in dark theme to match these values accordingly by specifying the correct hex values (I guess the values will be lighter then they are in the light theme)

# Conflicts: # CHANGELOG.md # packages/react/src/themes/teams/colors.ts # packages/react/src/themes/teams/siteVariables.ts

# Conflicts: # packages/react/src/themes/teams/components/Dropdown/dropdownVariables.ts

…ents' into feat/teams-theme-colors-arrangements

layershifter · 2019-02-14T11:29:42Z

packages/react/src/themes/teams-dark/siteVariables.ts

+export const gray08 = '#484644' // light theme gray02
+export const gray09 = '#3b3a39' // no mapping color
+export const gray10 = '#323130' // no mapping color
+export const gray14 = '#292828' // no mapping color


It feels like gray* and brand* there should extracted as part of color pallete

We have grey in the color palette, but the color are not matching these one, after the updates of the color palette, we may be able to reduce them. For the brand colors, currently I am mapping them to the primary, but we may change this to brand in the future.

layershifter · 2019-02-14T11:31:17Z

packages/react/src/themes/teams/components/Chat/chatMessageVariables.ts

  hasMention: false,
-  hasMentionColor: siteVars.orange04,
-  isImportantColor: siteVars.red,
+  hasMentionColor: siteVars.naturalColors.darkOrange[400],


Suggested change

hasMentionColor: siteVars.naturalColors.darkOrange[400],

hasMentionColor: siteVars.colors.darkOrange[400],

Actually I think this was producing TS errors, that's why I have the naturalColors. Let me check and will confirm

The problem is that the NaturalColorsStrict doesn't contain this color.

Created issue with proposed fixed: #898

layershifter · 2019-02-14T11:31:27Z

packages/react/src/themes/teams/components/Text/textVariables.ts

-  atMentionOtherColor: siteVariables.brand06,
-  atMentionMeColor: siteVariables.orange04,
+  atMentionOtherColor: siteVariables.colors.primary[500],
+  atMentionMeColor: siteVariables.naturalColors.darkOrange[400],


Suggested change

atMentionMeColor: siteVariables.naturalColors.darkOrange[400],

atMentionMeColor: siteVariables.colors.darkOrange[400],

layershifter · 2019-02-14T11:31:47Z

packages/react/src/themes/teams/components/Attachment/attachmentVariables.ts

+  textColor: siteVariables.colors.grey[900],

-  progressColor: siteVariables.green,
+  progressColor: siteVariables.naturalColors.lightGreen[900],


Suggested change

progressColor: siteVariables.naturalColors.lightGreen[900],

progressColor: siteVariables.colors.lightGreen[900],

layershifter · 2019-02-14T11:33:31Z

packages/react/src/themes/teams/components/Chat/chatMessageStyles.ts

-      siteVariables: { white },
+      siteVariables: {
+        colors: { white },
+      },


Actually, should be moved to chatMessageVariables

What do you mean? We have the white in the colors and not directly in the siteVariables

Sorry, updated comment, it's more clear

Got it, thanks!

layershifter · 2019-02-14T11:36:14Z

packages/react/src/themes/teams/siteVariables.ts

+export const gray08 = '#E1DFDD' // no mapping color
+export const gray09 = '#EDEBE9' // no mapping color
+export const gray10 = '#F3F2F1' // no mapping color
+export const gray14 = '#FAF9F8' // no mapping color


Have a mixed feeling about it. If we are trying to rid of colors on siteVariables directly, should we keep these?
May be add an evil prefix to them? __unsafe__gray14? 😈

This step is just a clean up, so that we can have clear picture of what colors are missing in the color palette. Then the design team can safely update the color palette, following the steps introduced in this PR. I would not want to introduce breaking changes now for these colors, as most likely they are going to be removed soon.

-wip

cee9e11

mnajdova requested review from dvdzkwsk, kuzhelov, layershifter, levithomason and miroslavstastny as code owners February 7, 2019 11:28

mnajdova added the 🚧 WIP label Feb 7, 2019

-fixes in styles/variables files

666b7ec

mnajdova changed the title ~~[DO NOT REVIEW YET]: teams theme colors arrangements~~ [DO NOT REVIEW]: teams theme colors arrangements Feb 7, 2019

mnajdova and others added 2 commits February 7, 2019 14:15

Merge branch 'master' into feat/teams-theme-colors-arrangements

80f2ef3

-updated changelog

3e11dc3

mnajdova changed the title ~~[DO NOT REVIEW]: teams theme colors arrangements~~ feat(Teams-theme): colors arrangements Feb 7, 2019

mnajdova changed the title ~~feat(Teams-theme): colors arrangements~~ feat(teams-theme): colors arrangements Feb 7, 2019

mnajdova added 🚀 ready for review and removed 🚧 WIP labels Feb 7, 2019

mnajdova changed the title ~~feat(teams-theme): colors arrangements~~ feat(teams-theme): clearing duplicate colors from siteVariables Feb 7, 2019

bmdalex approved these changes Feb 7, 2019

View reviewed changes

manajdov and others added 8 commits February 7, 2019 16:52

-adding mapping comments for the colors in dark theme

ab6be25

Merge branch 'master' into feat/teams-theme-colors-arrangements

1f437b5

# Conflicts: # CHANGELOG.md # packages/react/src/themes/teams/colors.ts # packages/react/src/themes/teams/siteVariables.ts

-fixed prototypes and HOC theme

4a41a38

-fixes in the dark theme

38e76c7

Merge branch 'master' into feat/teams-theme-colors-arrangements

9b6ca5a

Merge branch 'master' into feat/teams-theme-colors-arrangements

7634b26

# Conflicts: # packages/react/src/themes/teams/components/Dropdown/dropdownVariables.ts

-changed brand14 with colors.primary[100] in dropdownVariables

153f94d

Merge remote-tracking branch 'origin/feat/teams-theme-colors-arrangem…

f11515e

…ents' into feat/teams-theme-colors-arrangements

layershifter reviewed Feb 14, 2019

View reviewed changes

-updated change message variables

ab4880b

layershifter approved these changes Feb 14, 2019

View reviewed changes

Merge branch 'master' into feat/teams-theme-colors-arrangements

d4caa7d

mnajdova merged commit 36c51af into master Feb 14, 2019

miroslavstastny mentioned this pull request Feb 18, 2019

chore(dangerjs): add check for a changelog entry #918

Merged

layershifter deleted the feat/teams-theme-colors-arrangements branch February 18, 2019 14:50

	hasMentionColor: siteVars.naturalColors.darkOrange[400],
	hasMentionColor: siteVars.colors.darkOrange[400],

	atMentionMeColor: siteVariables.naturalColors.darkOrange[400],
	atMentionMeColor: siteVariables.colors.darkOrange[400],

	progressColor: siteVariables.naturalColors.lightGreen[900],
	progressColor: siteVariables.colors.lightGreen[900],

Conversation

mnajdova commented Feb 7, 2019 • edited by kuzhelov Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description of the problem

Goal

BREAKING CHANGES (Teams theme)

Uh oh!

codecov bot commented Feb 7, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

bmdalex 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!

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!

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!

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 Feb 14, 2019 • 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!

layershifter Feb 14, 2019 • 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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

mnajdova commented Feb 7, 2019 •

edited by kuzhelov

Loading

codecov bot commented Feb 7, 2019 •

edited

Loading

layershifter Feb 14, 2019 •

edited

Loading

layershifter Feb 14, 2019 •

edited

Loading