-
Notifications
You must be signed in to change notification settings - Fork 55
feat(teams-theme): clearing duplicate colors from siteVariables #858
Conversation
Codecov Report
@@ 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
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
YAAASSS!!
300: '#8F90C1', | ||
400: '#6E70AE', | ||
500: '#6264A7', | ||
500: '#6264A7', // siteVariables.brand, siteVariables.brand06 (same color?) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, it's the same color
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for pointing out :D
@@ -19,7 +19,7 @@ export const brand02 = '#e2e2f6' | |||
export const brand04 = '#bdbde6' | |||
export const brand06 = '#a6a7dc' | |||
export const brand08 = '#8b8cc7' | |||
export const brand12 = brand | |||
export const brand12 = colors.primary[500] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It feels like gray*
and brand*
there should extracted as part of color pallete
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
border: 'none', | ||
badgeShadow: siteVars.shadowLevel1Darker, | ||
isImportant: false, | ||
hasMention: false, | ||
hasMentionColor: siteVars.orange04, | ||
isImportantColor: siteVars.red, | ||
hasMentionColor: siteVars.naturalColors.darkOrange[400], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hasMentionColor: siteVars.naturalColors.darkOrange[400], | |
hasMentionColor: siteVars.colors.darkOrange[400], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually I think this was producing TS errors, that's why I have the naturalColors
. Let me check and will confirm
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The problem is that the NaturalColorsStrict
doesn't contain this color.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Created issue with proposed fixed: #898
atMentionOtherColor: siteVariables.brand06, | ||
atMentionMeColor: siteVariables.orange04, | ||
atMentionOtherColor: siteVariables.colors.primary[500], | ||
atMentionMeColor: siteVariables.naturalColors.darkOrange[400], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
atMentionMeColor: siteVariables.naturalColors.darkOrange[400], | |
atMentionMeColor: siteVariables.colors.darkOrange[400], |
|
||
progressColor: siteVariables.green, | ||
progressColor: siteVariables.naturalColors.lightGreen[900], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
progressColor: siteVariables.naturalColors.lightGreen[900], | |
progressColor: siteVariables.colors.lightGreen[900], |
siteVariables: { white }, | ||
siteVariables: { | ||
colors: { white }, | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, should be moved to chatMessageVariables
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you mean? We have the white in the colors
and not directly in the siteVariables
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, updated comment, it's more clear
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it, thanks!
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
? 😈
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Description of the problem
In Teams theme we are exporting colors in different ways
colors
object (containing the natural, emphasis, primitive colors)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 assiteVariables.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 thesiteVariables.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
andsiteVariables.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 thesiteVariables
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 thecolors
object. From here then, we can have clear picture of which colors defined in thesiteVariables
, 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:In addition, the value of the
siteVariable.colors.black
was changed fromsiteVariables.colors.grey[900] (#252424)
to#000
. If the intention of the user was to have the dark grey, then please replace the usage withsiteVariables.colors.grey[900]
.