-
Notifications
You must be signed in to change notification settings - Fork 55
fix: make theme variables and styles extensible #292
Conversation
7dbb692
to
fe64549
Compare
Codecov Report
@@ Coverage Diff @@
## master #292 +/- ##
=======================================
Coverage 89.54% 89.54%
=======================================
Files 62 62
Lines 1177 1177
Branches 175 152 -23
=======================================
Hits 1054 1054
Misses 121 121
Partials 2 2 Continue to review full report at Codecov.
|
daa2cb2
to
bac6ea8
Compare
@@ -60,6 +60,9 @@ export interface ICSSPseudoElementStyle extends ICSSInJSStyle { | |||
} | |||
|
|||
export interface ICSSInJSStyle extends React.CSSProperties { | |||
// TODO Questionable: how else would users target their own children? | |||
[key: string]: any |
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 it wrong to make this [key: string]: CssInJssStyle
? I am a bit worried with the any there..
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.
Ah, yep. Good call.
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 is actually quite a bit trickier. Not every string property is an ICSSInJSStyle. Some have limited values, such as speak
, so they cannot be typed as ICSSInJSStyle.
Since this is currently blocking and broken, let's continue with a more permissive object (as is) for now. We can always improve typings.
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! Approved.
@@ -163,72 +166,142 @@ export interface IThemePrepared { | |||
} | |||
|
|||
export interface IThemeComponentStylesInput { | |||
[key: string]: any | |||
|
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.
As in the previous comment, can we make this: [key: string]: IComponentPartStylesInput
? What else can be the value of this?
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, here we can since all keys have the same value type. Same with IThemeComponentStylesPrepared below which I have updated as well.
bac6ea8
to
cda5693
Compare
Text?: IComponentPartStylesPrepared | ||
} | ||
|
||
export interface IThemeComponentVariablesInput { | ||
[key: string]: any |
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.
I guesss the same can be applied here as well as in the IThemeComponentVariablesPrepared
? [key: string]: ComponentVariablesInput
& [key: string]: ComponentVariablesPrepared
? Otherwise we have some typings errors already..
Currently, consumers cannot add custom component display names to a theme's
componentVariables
andcomponentStyles
. This is a problem as they will need to add their component's to the theme as well.I've also added the unlisted component display names the theme.