-
Notifications
You must be signed in to change notification settings - Fork 55
fix(Avatar): Adding dark and contrast theme files for Avatar and Status Indicator. #373
Conversation
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)
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.
please, just update CHANGELOG before merging 👍
Modified Avatar to support Dark and Contrast themes
Before:
After: