Remove classes optional typings#3894
Conversation
|
@dzearing I think this isn't as useful as hoped though and basically doesn't change much. Because of the dynamic property creation it seems it'll be difficult to type |
|
@Markionium is there a need to make ColorClassNames a partial? |
|
@dzearing I was hoping it wasn't but TS won't allow me to assign export const ColorClassNames: Partial<IColorClassNames> = {};I tried to "fix" it by reducing instead of building it up with a TS is technically doing the correct thing since removing a color from the I also considered changing the ColorClassNames into something like the following, so we could do something like below, but it leaves me with the same issue around the dynamic properties. interface ColorClasses {
text: string;
textHover: string;
background: string;
backgroundHover: string;
border: string;
borderHover: string;
}
export type IColorClassesNames = {[key in keyof IPalette]: ColorClasses};PS: It's not such a big issue as i resolved on our side, but perhaps still worth it to fix it somehow? |
| } | ||
|
|
||
| export const ColorClassNames: IColorClassNames = {}; | ||
| export const ColorClassNames: Partial<IColorClassNames> = {}; |
There was a problem hiding this comment.
Shouldn't this be:
export const ColorClassNames: IColorClassNames = {} as IColorClassNames;Then in your code, you don't need to treat it as an non-optional?
| */ | ||
| function _defineGetter( | ||
| obj: IColorClassNames, | ||
| obj: Partial<IColorClassNames>, |
There was a problem hiding this comment.
You don't need this if you do what I said above.
dzearing
left a comment
There was a problem hiding this comment.
If you tweak it a little, I think it will do what you want.
Pull request checklist
$ npm run changeDescription of changes
Changes
ColorClassNamesto usePartial<T>Focus areas to test
(optional)