chore(colors): Improve colors typings per component variables #1340

mnajdova · 2019-05-15T11:06:17Z

Things addressed in this PR:

In teams theme, we want to scope the color used to the ones actually defined in the theme (introduced generic for this)
We want from component's variables to export the parts of the color scheme only the areas actually used in the component. For example for the Label component, we are interested only in the background and foreground colors. (introduced two more generics for this, as well as stricter version of ones of the existing ones' introduced helper method: pickValuesFromColorScheme for making this easier for developers)
removed getColorScheme, and used getColorSchemeKey in order not to complicate too much the functions with generics that will have to be added by devs.
renamed emphasisColors to contextualColors in Teams theme, as we have only the brand value there (we don't have primary and secondary colors) - open for other possible names here.
added transparent colors in the color palette, those were added later in the process
added Not defined information on the Color Box for being more clear, rather then just showing transparent box

…s of a component

codecov · 2019-05-15T11:53:35Z

Codecov Report

Merging #1340 into master will increase coverage by 0.05%.
The diff coverage is 69.23%.

@@            Coverage Diff             @@
##           master    #1340      +/-   ##
==========================================
+ Coverage   72.99%   73.05%   +0.05%     
==========================================
  Files         776      777       +1     
  Lines        5828     5848      +20     
  Branches     1706     1706              
==========================================
+ Hits         4254     4272      +18     
- Misses       1568     1570       +2     
  Partials        6        6

Impacted Files	Coverage Δ
...rk/components/Header/headerDescriptionVariables.ts	`0% <ø> (ø)`	⬆️
packages/react/src/index.ts	`100% <ø> (ø)`	⬆️
...emes/teams-dark/components/Label/labelVariables.ts	`0% <ø> (ø)`	⬆️
...c/themes/teams/components/Divider/dividerStyles.ts	`16.66% <ø> (ø)`	⬆️
.../teams-dark/components/Divider/dividerVariables.ts	`0% <ø> (ø)`	⬆️
packages/react/src/themes/teams/siteVariables.ts	`100% <ø> (ø)`	⬆️
...ams-high-contrast/components/Menu/menuVariables.ts	`0% <ø> (ø)`	⬆️
...themes/teams-dark/components/Menu/menuVariables.ts	`0% <ø> (ø)`	⬆️
...t/src/themes/base/components/Text/textVariables.ts	`0% <ø> (ø)`	⬆️
...s-high-contrast/components/Label/labelVariables.ts	`0% <ø> (ø)`	⬆️
... and 16 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 e000bc8...62eb82b. Read the comment docs.

packages/react/src/themes/teams/components/Label/labelVariables.ts

kuzhelov · 2019-05-15T12:43:04Z

packages/react/src/themes/teams/components/Label/labelVariables.ts

+      background: color,
+      foreground: 'rgb(232, 232, 232)',
+    },
+    brand: {


not sure if you were going to address this by the set of changes provided, but as a client I would expect that brand would appear under props of LabelColorSchemeMapping, so that at least types would suggest me that this is a safe thing to use:

The problem is that now, we are using one ColorNames type, but this should be redefine for teams theme (we don't have all colors that the typings suggest, and we have few more. Will introduce this changes too.

Introduced TeamsColorNames, a bit scared as how these typings with generics are looking now :S I am open for ides for restructuring them. Will try on my own to see how can I improve these.

packages/react/src/themes/colorUtils.ts

kuzhelov · 2019-05-15T12:52:57Z

packages/react/src/themes/teams/components/Label/labelVariables.ts

+      foreground: 'rgb(232, 232, 232)',
+    },
+    brand: {
+      background: siteVars.colorScheme.brand.foreground4,


not sure if this PR is going to address this, but as a client might see another issue with types. Here you might see that types suggest that foreground prop is guaranteed to be defined for each color of colorSheme.

However, given this code, it is clear that value might be omitted - this is the case of brand color for Label's color scheme, where foreground is absent.

It would be great if our types will suggest the fact that value of area props (in our example, foreground) is, in fact, optional

The color scheme defined for the teams theme, already have all these values defined. In the labelVariables, we are just overriding some of those. Basically there is no need to override the brand foreground hover, that's why it is not defined.

mnajdova · 2019-05-16T10:54:41Z

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

+import { ColorPalette, PrimitiveColors, ColorSchemeMapping, ColorVariants } from '../types'
+import { ColorScheme } from 'src/themes/types'
+
+export type TeamsContextualColors = {


Interfaces added for the Team's specific colors

-added transparent colors in the color palette

mnajdova · 2019-05-16T11:28:04Z

packages/react/src/themes/teams/components/Label/labelVariables.ts

+type LabelColorSchemeMapping = StrictColorSchemeMapping<
+  StrictColorScheme<LabelColorComponentAreas>,
+  TeamsColorNames
+>


I want us to decide if this is the right approach for defining these, and I will extract this to a separate utility and reuse it in other component variables files.

are we expecting this TeamsColorNames set to change based on the component type? I think, we might start from exporting simpler StrictTeamsColorSchemeMapping<TComponentAreas> type that would do this scaffolding in the background. This will make type interface being easier to client.

Also, we might want to rename this type to something like TeamsReducedSchemeMapping<TComponentAreas>

I would expect that those will be different across different component, as the text have much more color values then the menu for example. (The menu supports only default and brand colors and we want to use that as typings in the styles.)

mnajdova · 2019-05-16T11:42:14Z

packages/react/src/themes/teams/components/Label/labelVariables.ts


 export interface LabelVariables {
-  colorScheme: ColorSchemeMapping
+  colorScheme: LabelColorSchemeMapping


This may be a problem when we are using here some base theme interface and want to extend this, as this prop should have a different type..

just to be sure this is not overlooked - could you, please, suggest an example of this (like, some pseudo-code)? Because at the moment I think that it would be normal to extend this Teams interface in that case - and semantically that would mean that now this colors' set is dependent on colors object (or, as equivalent, its type) provided by base theme.

Sure, let me provide an example. The text component in the base themes, specifies some color names, that may won't exists in the Teams theme (like blue for example). That means, that the interface that we use here will be different. For example:

//base theme: interface TextVariables { colorScheme: ColorSchemeMapping<ColorScheme, ColorNames> } //Teams theme: interface TextVariables { colorScheme: ColorSchemeMapping<ColorScheme, TeamsColorNames> }

docs/src/components/ColorBox.tsx

docs/src/views/ColorPalette.tsx

packages/react/src/themes/colorUtils.ts

kuzhelov · 2019-05-17T12:04:12Z

packages/react/src/themes/colorUtils.ts

@@ -16,3 +21,23 @@ export const extendColorScheme = (
  })
  return result
 }
+
+export function pickValuesFromColorScheme<T extends Partial<ComponentAreaName>>(


maybe simpler pickColors name, given that arguments are descriptive enough?

Hmm, it's not about picking colors is more about picking areas (like foreground, background)... So maybe pickColorAreas would be better?

kuzhelov · 2019-05-17T12:14:05Z

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

+import { ColorScheme } from 'src/themes/types'
+
+export type TeamsContextualColors = {
+  brand: ColorVariants


on ColorVariants - currently this type is defined as type that any Stardust theme could use. However, we don't have any piece of Stardust code that relies on this structure (actually, good thing, as different themes might use different schemes to organise colors), and Teams is the only theme which uses it.

I think that it is logical to move this type from common to the types of Teams theme

The base theme is using these typings as well. Yes is not required by any react specific file, but still is used in base theme..

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

kuzhelov · 2019-05-17T12:24:23Z

packages/react/src/themes/teams/components/Label/labelVariables.ts

+type LabelColorSchemeMapping = StrictColorSchemeMapping<
+  StrictColorScheme<LabelColorComponentAreas>,
+  TeamsColorNames
+>


are we expecting this TeamsColorNames set to change based on the component type? I think, we might start from exporting simpler StrictTeamsColorSchemeMapping<TComponentAreas> type that would do this scaffolding in the background. This will make type interface being easier to client.

Also, we might want to rename this type to something like TeamsReducedSchemeMapping<TComponentAreas>

kuzhelov · 2019-05-17T12:42:10Z

packages/react/src/themes/teams/components/Label/labelVariables.ts


 export interface LabelVariables {
-  colorScheme: ColorSchemeMapping
+  colorScheme: LabelColorSchemeMapping


just to be sure this is not overlooked - could you, please, suggest an example of this (like, some pseudo-code)? Because at the moment I think that it would be normal to extend this Teams interface in that case - and semantically that would mean that now this colors' set is dependent on colors object (or, as equivalent, its type) provided by base theme.

-removed transparent colors

bmdalex

I'd need to deep dive to understand all the changes, let's chat in person

packages/react/src/themes/teams/components/Label/labelVariables.ts

packages/react/src/themes/teams/components/Menu/menuDividerStyles.ts

# Conflicts: # docs/src/views/ColorPalette.tsx # packages/react/src/themes/teams/colors.ts # packages/react/src/themes/teams/components/Label/labelVariables.ts # packages/react/src/themes/teams/siteVariables.ts

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

bmdalex

👍 looks good to me, feel free to merge once comments from other reviewers are resolved

docs/src/components/ColorBox.tsx

packages/react/src/themes/types.ts

-replaced header prop in the ColorBox with copyToClipboardIcon

# Conflicts: # CHANGELOG.md

manajdov added 2 commits May 15, 2019 13:02

-added mechanism for extracting only usable parts in the color scheme…

dfaa8f1

…s of a component

-remove comments

0237dff

mnajdova requested review from dvdzkwsk, kuzhelov, layershifter, levithomason and miroslavstastny as code owners May 15, 2019 11:06

-added type in the tuple usage

6e5cbda

mnajdova commented May 15, 2019

View reviewed changes

packages/react/src/themes/teams/components/Label/labelVariables.ts Outdated Show resolved Hide resolved

kuzhelov reviewed May 15, 2019

View reviewed changes

packages/react/src/themes/teams/components/Label/labelVariables.ts Outdated Show resolved Hide resolved

kuzhelov reviewed May 15, 2019

View reviewed changes

packages/react/src/themes/colorUtils.ts Outdated Show resolved Hide resolved

kuzhelov reviewed May 15, 2019

View reviewed changes

manajdov added 2 commits May 15, 2019 15:42

-fixed some issues

a3988de

-added TeamsColorNames

6f6f662

mnajdova commented May 16, 2019

View reviewed changes

manajdov added 2 commits May 16, 2019 12:55

-renamed emphasisColors to contextualColors

2d1c670

-renamed emphasisColors usages to contextualColors

c4b554f

-added transparent colors in the color palette

mnajdova commented May 16, 2019

View reviewed changes

mnajdova changed the title ~~[WIP] chore(colors): Improve colors typings per component variables~~ chore(colors): Improve colors typings per component variables May 16, 2019

mnajdova added the 🚀 ready for review label May 16, 2019

mnajdova commented May 16, 2019

View reviewed changes

kuzhelov reviewed May 17, 2019

View reviewed changes

docs/src/components/ColorBox.tsx Outdated Show resolved Hide resolved

kuzhelov reviewed May 17, 2019

View reviewed changes

manajdov added 3 commits May 17, 2019 15:33

-changed param name

b515589

-added unit tests for the new color util func

460f37f

-removed transparent colors

-remove transparentColors from the siteVariabels as well

238aa0a

bmdalex reviewed May 22, 2019

View reviewed changes

packages/react/src/themes/teams/components/Label/labelVariables.ts Outdated Show resolved Hide resolved

packages/react/src/themes/teams/components/Menu/menuDividerStyles.ts Outdated Show resolved Hide resolved

Merge branch 'master' into feat/improve-color-mechanisms

923cab9

# Conflicts: # docs/src/views/ColorPalette.tsx # packages/react/src/themes/teams/colors.ts # packages/react/src/themes/teams/components/Label/labelVariables.ts # packages/react/src/themes/teams/siteVariables.ts

manajdov added 6 commits May 22, 2019 11:25

-fixed color scheme as header

4851b6c

-extract generating component areas to a separate util

214e293

-updated menu variables

50f5f69

-fixed relative improts

bfd202e

-added getColorScheme in all menu styles

e7dcbe4

-converted segment

f5375f7

mnajdova commented May 22, 2019

View reviewed changes

packages/react/src/themes/teams/components/Text/textVariables.ts Outdated Show resolved Hide resolved

manajdov added 4 commits May 22, 2019 16:05

-moved other components' color schemes to the right typings

e427e9d

-fixed imports

0b61316

-fixed relative import

107799d

-fixed menu regressions

d95301e

kuzhelov reviewed May 23, 2019

View reviewed changes

manajdov added 3 commits May 23, 2019 14:09

-addressed PR comments

63a1850

Merge branch 'master' into feat/improve-color-mechanisms

6d14356

-updated changelog

e14f08c

bmdalex approved these changes May 23, 2019

View reviewed changes

kuzhelov reviewed May 23, 2019

View reviewed changes

docs/src/components/ColorBox.tsx Outdated Show resolved Hide resolved

kuzhelov reviewed May 23, 2019

View reviewed changes

packages/react/src/themes/types.ts Outdated Show resolved Hide resolved

kuzhelov approved these changes May 23, 2019

View reviewed changes

manajdov and others added 5 commits May 23, 2019 19:04

-extracted Teams types

80a2874

-replaced header prop in the ColorBox with copyToClipboardIcon

Merge branch 'master' into feat/improve-color-mechanisms

93010e3

# Conflicts: # CHANGELOG.md

-fixed getting divider colors

39f0c96

Merge branch 'master' into feat/improve-color-mechanisms

c81305d

-reverted divider styles changes

62eb82b

mnajdova merged commit b3782cf into master May 23, 2019

delete-merged-branch bot deleted the feat/improve-color-mechanisms branch May 23, 2019 18:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore(colors): Improve colors typings per component variables #1340

chore(colors): Improve colors typings per component variables #1340

mnajdova commented May 15, 2019 •

edited

codecov bot commented May 15, 2019 •

edited

kuzhelov May 15, 2019

mnajdova May 15, 2019

mnajdova May 16, 2019

kuzhelov May 15, 2019 •

edited

mnajdova May 15, 2019

mnajdova May 16, 2019

mnajdova May 16, 2019

kuzhelov May 17, 2019

mnajdova May 17, 2019

mnajdova May 16, 2019

kuzhelov May 17, 2019

mnajdova May 17, 2019

kuzhelov May 17, 2019

mnajdova May 17, 2019

kuzhelov May 17, 2019

mnajdova May 17, 2019

kuzhelov May 17, 2019

kuzhelov May 17, 2019

bmdalex left a comment

bmdalex left a comment

chore(colors): Improve colors typings per component variables #1340

chore(colors): Improve colors typings per component variables #1340

Conversation

mnajdova commented May 15, 2019 • edited

codecov bot commented May 15, 2019 • edited

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kuzhelov May 15, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bmdalex left a comment

Choose a reason for hiding this comment

bmdalex left a comment

Choose a reason for hiding this comment

mnajdova commented May 15, 2019 •

edited

codecov bot commented May 15, 2019 •

edited

kuzhelov May 15, 2019 •

edited