feat(theme): updating color palette and scheme #1069
Conversation
-updated on the menu styles
mnajdova
requested review from
dvdzkwsk,
kuzhelov,
layershifter,
levithomason and
miroslavstastny
as code owners
Codecov Report
@@ Coverage Diff @@
## master #1069 +/- ##
==========================================
- Coverage 72.85% 72.24% -0.61%
==========================================
Files 747 752 +5
Lines 5658 5614 -44
Branches 1655 1671 +16
==========================================
- Hits 4122 4056 -66
- Misses 1529 1551 +22
Partials 7 7
Continue to review full report at Codecov.
|
-updates on the styles of the segment component
-fixed compiler issues
-improved menu color example
-menu divider color fix
-renamed areaDefault values from the color scheme with just area
# Conflicts: # packages/react/src/components/Menu/Menu.tsx
| backgroundFocus1: siteVars.colors.brand[300], | ||
| }, | ||
| }), | ||
| color: siteVars.colors.grey[500], |
There was a problem hiding this comment.
Is there a plan for future refactoring of the color palette/variables?
I am confused with which variable is used for what and when:
colorScheme.foregroundis never used, we usevariables.color- for active primary item background goes from color scheme (
colorsScheme.backgroundActive1) and can be overriden by variable (variables.backgroundColorActive), color goes from color scheme (colorScheme.foregroundActive) but cannot be overridden. v.backgroundColorActive || colorScheme.backgroundActive1and the variable defaults toundefined, butv.underlinedBorderColor || colorScheme.backgroundActive1and the variable is set?
There was a problem hiding this comment.
Yep, there is a plan for that, but if we were about to do that now, it would took a long time. We will go component by component, and we will update the styles to use the colorScheme values. Usually, they will be used in the following way:
{
root: ({variables}) => {
const colors = getColorScheme(variables.colorScheme, color, primary);
return {
...,
color: v.color || colors.foregroundColor,
}
},
...
}
The variables would default to undefined, and we need to come up with a way of how to show it in the docs.
For colors that should never change (even if different color or primary prop is provided), we can have directly value in the variable.
…s, typos in the Color guide
|
|
||
| export interface TextVariables { | ||
| colors: ColorValues<string> | ||
| colorScheme: ColorSchemeMapping |
There was a problem hiding this comment.
this common type implies that supported ColorNames (in case if color props is supported by component) should be the same for any component of the app. In other words, if there is pink in color names, it should be supported by all components in corresponding schemes.
Won't it be a problem if, for instance, we would need to support extended range of colors for some particular component? Or this case is not considered intentional, as design spec rules explicitly won't allow this to happen? What would be our options to support this case, if necessary?
I see extendColorScheme method, so I might expect that we will introduce only minimal set of color options for base scheme, and will allow components extend this set of necessary, without affecting others - am I right on that?
There was a problem hiding this comment.
also, related question: currently, for any component that supports color prop, it is possible to introduce the following markup :
<Text color={any_color_from_color_names} />
<Segment ... > // same thingWondering of we would like to introduce this much flexibility to the client - as, essentially, only subset of these color variants of Text (or Segment) components is supposed to be used in the app.
Of course, client might omit defining colors, and then default one will be applied on render - but then markup will be misleading.
Could you, please, suggest our plans to handle this situations?
There was a problem hiding this comment.
Very good question. As part of this PR, I just scoped this color value for the prop, to the colors we actually have, and I am defaulting this to the default styles if the color is not in this set. Ideally, we would have color values available defined per component, but this was still not specified by design. After we have these specs, we can update the component styles to handle only the colors they are suppose to. These changes were indeed just to unify the colors, and return default styles if something not expected is provided.
| @@ -1,8 +1,7 @@ | |||
| import { ColorValues } from '../../../types' | |||
There was a problem hiding this comment.
could you, please, walk me through client's workflow on this. So, I would like to introduce Foo component that is able to color itself with theme being switched. What is the source I need to use colors from?
Am I right that I am supposed to take a look on the Design Spec and use colors from siteVariables, as a first thing? Like
// variables, default theme
return {
atMentionMeColor: siteVariables.colors.pink[500], // those are palette colors
atMentionOtherColor: siteVariables.colors.grey[600],
importantColor: siteVariables.colors.red[500]
...
}Then, to override colors for different themes, I need to override corresponding variables, according to the Design Spec:
// variables, Dark Theme
return {
atMentionMeColor: siteVariables.colors.red[500], // overriding with palette color
...
}And then, if I would like to support color variants for my component (i.e. color prop), then this is the moment when I need to plug colorScheme, right - otherwise I don't need it?
// variables, default theme
return {
colorScheme: siteVariables.colorScheme,
atMentionMeColor: siteVariables.colors.pink[500], // those are palette colors
atMentionOtherColor: siteVariables.colors.grey[600],
importantColor: siteVariables.colors.red[500]
...
}There was a problem hiding this comment.
Even if the component doesn't support color, the color scheme defines the colors that should be used across the app. Design should specify in the specs which colors should be used for this component, for example:
// variables, default theme
return {
atMentionMeColor: siteVariables.colorScheme.pink.foreground, // those are palette colors
atMentionOtherColor: siteVariables.colorScheme.default.foreground1,
importantColor: siteVariables.colorScheme.red.foreground,
...
}
This colors will be mapped correctly in all different themes, so the user won't have to do the overrides. Currently we have only the default and primary color schemes, but design is working on defining the schemes for the other colors as well.
On the other hand, if the component supports the color prop, you are right, they can leverage further the color scheme, by picking the color provided by the color prop.
There was a problem hiding this comment.
got it - so, essentially, the vector of Design Spec rules reduces to the following:
- wherever it is possible, use colors from color sheme
- if rules of color scheme doesn't apply, define color from color palatte
Am I right here?
There was a problem hiding this comment.
wherever it is possible, use colors from color sheme
Yep, totally right
if rules of color scheme doesn't apply, define color from color palatte
Yep, and override them to the correct values in all different themes.
NOTE: these changes are mostly scoped to Teams theme only. The only core Stardust functionality change, is that the
colorsobject is not available anymore in the style functions, the variables should be the only place defining values for the styles.Colors updates
First of all, let me start with the motivation for starting this work on the colors. We need to deliver the following functionalities in the definition of the styles:
gray04, orprimary08) which doesn’t mean anything in the mind of the developer. We are trying to move more towards names like brand foreground color, or brand foreground hover color, that will express better the intent of its usage.With these in mind, we will introduce two things for the Light, Dark and High Contrast Teams themes.
The changes applied in the PR
We introduced one global color palette, created by design, that contains all colors that should be used in the Teams application. This will not be changed across different themes, it will always be the same object.
Apart from this, we created three color schemes, one in each theme, which are identical in their structure (so people can pick values from them safely), but the actual values of the picked token will have different values for colors. For example, this is how the brand part of the color scheme, looks in light, hoc and dark theme:
What this means is that, if a developer use in their styles use:
colorScheme.primary.backgroundHover, it will correctly be mapped to #8DBDE6 in light/dark theme, and to #000 in hoc theme. This is Stardust’s proposal of how the design tokens can be organized, we still have to work with design to conclude on this. As an example of possible change in the future is that, we may introduce inverted schemas for each of the themes.Fixes #898 and fixes #707
Introducing colors in components.
Let's say that you want to introduce Foo component that is able to color itself with theme being switched. How should you start with this:
This colors will be mapped correctly in all different themes, so the user won't have to do the overrides.
Then, to override colors for different themes, according to the Design Spec:
On the other hand, if you are developing component that should support the color prop, then you may use directly the color scheme, pick the color that is provided by the prop, and use directly the values from the color scheme (again those should be approved by design for the component being developed).
BREAKING CHANGES:
I will introduce here the mappings for the colors of all the three Teams themes to the corresponding value in the global color palette.
Light theme
Dark theme
HC theme
Previous color palette mappings
Renamed variables
Other breaking changes
color='primary'should be replace withcolor='brand'colorsobject is not available in the style function anymore, thecolorSchemeobject should be used instead.