Skip to content

Conversation

@Markionium
Copy link
Member

Pull request checklist

  • Addresses an existing issue: Fixes #0000
  • Include a change request file using $ npm run change

Description of changes

Changes ColorClassNames to use Partial<T>

Focus areas to test

(optional)

@Markionium
Copy link
Member Author

@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 ColorClassNames with non-optionals. Even though we know that all the classNames will be there :)

@dzearing
Copy link
Member

dzearing commented Feb 7, 2018

@Markionium is there a need to make ColorClassNames a partial?

@Markionium
Copy link
Member Author

Markionium commented Feb 7, 2018

@dzearing I was hoping it wasn't but TS won't allow me to assign {} to ColorClassNames. Since we know all the props will be there we could do an as ColorClassNames but thats not great either.

export const ColorClassNames: Partial<IColorClassNames> = {};

I tried to "fix" it by reducing instead of building it up with a for in loop.

TS is technically doing the correct thing since removing a color from the DefaultPallete would result into non-existent properties. (Afaik it isn't possible to have dynamic types based on the runtime created strings? It might be that due to my relatively short experience with TS I'm missing something)

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> = {};
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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>,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You don't need this if you do what I said above.

Copy link
Member

@dzearing dzearing left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you tweak it a little, I think it will do what you want.

@dzearing dzearing self-assigned this Feb 13, 2018
@dzearing dzearing merged commit 8b573a6 into microsoft:master Feb 14, 2018
@microsoft microsoft locked as resolved and limited conversation to collaborators Aug 31, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants