feat(colors): add new set for unified ramp of category colors in Teams theme #1711
Conversation
natashamayurshah
requested review from
dvdzkwsk,
kuzhelov,
layershifter,
levithomason,
miroslavstastny and
mnajdova
as code owners
Codecov Report
@@ Coverage Diff @@
## master #1711 +/- ##
==========================================
+ Coverage 69.78% 69.82% +0.03%
==========================================
Files 872 871 -1
Lines 7503 7506 +3
Branches 2183 2180 -3
==========================================
+ Hits 5236 5241 +5
+ Misses 2259 2257 -2
Partials 8 8
Continue to review full report at Codecov.
|
| } | ||
| } | ||
|
|
||
| export const categoryColorSchemes: CategoryColorSchemeMapping = { |
There was a problem hiding this comment.
Let's preserve the previous naming convention and rename this to categoryColorScheme
|
|
||
| const createCategoryColorScheme = (color: string, customValues = {}) => { | ||
| return { | ||
| foreground1: categoryColors[color][250], |
There was a problem hiding this comment.
Let's replaced foreground1 with foreground and foreground2 with foreground1
There was a problem hiding this comment.
Also, is there a plan for the future, more of these colors to be added in the color scheme? Not sure why we have so many values in the color palette, but we clearly only use a subset of them..
There was a problem hiding this comment.
Moving the second comment discussion into the conversation with Sue Nguyen.
packages/react/src/themes/types.ts
Outdated
| /** | ||
| * A type for a predefined category colors. | ||
| */ | ||
| type CategoryColorsStrict = Partial<{ |
There was a problem hiding this comment.
If this is supposed to be Strict, let's remove the Partial from the definition. If it is not suppose to be Strict, let's change the name to CategoryColors.
There was a problem hiding this comment.
Also these typings are specific to team theme, so we may move them to packages/react/src/themes/teams/types.ts
packages/react/src/themes/types.ts
Outdated
| /** | ||
| * A type for an extendable set of CategoryColorNames properties of type T | ||
| */ | ||
| type CategoryColorValues< |
There was a problem hiding this comment.
ColorValues is already a generic, can't we just reuse it with different params
packages/react/src/themes/types.ts
Outdated
| Record<T, string> | ||
| > | ||
|
|
||
| export type CategoryColorSchemeMapping< |
There was a problem hiding this comment.
We may also reuse the generics from the original ColorSchemeMapping type
|
Final thing :) Let's add entry in the CHANGELOG.md file under
|
mnajdova
changed the title
Add all variants for the new set of category colors.