fix(Avatar): Adding dark and contrast theme files for Avatar and Status Indicator.#373
fix(Avatar): Adding dark and contrast theme files for Avatar and Status Indicator.#373
Conversation
bcalvery
requested review from
dvdzkwsk,
kuzhelov,
levithomason,
miroslavstastny and
mnajdova
as code owners
Codecov Report
@@ Coverage Diff @@
## master #373 +/- ##
==========================================
- Coverage 91.77% 91.64% -0.13%
==========================================
Files 41 41
Lines 1325 1341 +16
Branches 194 197 +3
==========================================
+ Hits 1216 1229 +13
- Misses 105 108 +3
Partials 4 4
Continue to review full report at Codecov.
|
…x/avatar-darktheme
…x/avatar-darktheme
giving contrast theme status colors. fixing contrast theme status border width.
…x/avatar-darktheme
src/components/Status/Status.tsx
Outdated
| variables: { color: 'white' }, // This is temporary. There is a ToDo to use icon's text/fill color for box-shadow, currently it uses color | ||
| styles: styles.icon, | ||
| variables: variables.icon, | ||
| className: classes.icon, |
There was a problem hiding this comment.
these classes.icon shouldn't be passed, as classes.icon contain already processed styles that are transformed to classes (styles.icon -> classes.icon). We should just pass original styles.icon and let Icon component process them by itself. This is critical to ensure that these styles.icon will be correctly merged with those that could be provided to icon shorthand in a following way:
<Status icon={{ styles: ... }} />
|
|
||
| const status = { color: 'green', icon: 'check', title: 'Available' } | ||
|
|
||
| const AvatarUsageExampleShorthand = () => ( |
There was a problem hiding this comment.
just a note for future: for all the components we have this section where examples that are rather theme-specific are provided. It is a clear indicator that we should think about mechanisms for theme developers to define custom docs section where theme-specific examples will be provided.
| @@ -0,0 +1,40 @@ | |||
| import { pxToRem } from '../../../../lib' | |||
There was a problem hiding this comment.
note that these styles are one-to-one the styles that are used for the default theme. To eliminate code repetition we could just reuse the ones that are used for default theme.
const darkThemeOverrides = {
componentVariables: {
...
Avatar: { /* .. overriden variables for Avatar component .. */ }
}
}
..
const darkTheme = mergeThemes(defaultTheme, darkThemeOverrides)
| @@ -0,0 +1,9 @@ | |||
| export interface IAvatarVariables { | |||
There was a problem hiding this comment.
the type declared for default theme should be reused - this would guarantee consistency of variables across different theme variants
| @@ -0,0 +1,63 @@ | |||
| import { pxToRem } from '../../../../lib' | |||
There was a problem hiding this comment.
same story here - we shouldn't replicate the same styles, but rather introduce overrides that are specific to dark theme
There was a problem hiding this comment.
there is lots of code duplication at the moment, the ones that are caused by literal copying the styles and variables of default theme into dark and high-contrast one. Lets address that by using the following approach:
const darkThemeOverrides = {
componentVariables: {
...
Avatar: { /* .. overriden variables for Avatar component .. */ }
}
}
..
const darkTheme = mergeThemes(defaultTheme, darkThemeOverrides)
kuzhelov
added
needs author feedback
kuzhelov
left a comment
kuzhelov
left a comment
There was a problem hiding this comment.
please, just update CHANGELOG before merging 👍
4 participants
Modified Avatar to support Dark and Contrast themes
Before:
After:
