Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[theme] Correct augmentColor TypeScript definition #13376

Merged
merged 2 commits into from
Oct 25, 2018
Merged

[theme] Correct augmentColor TypeScript definition #13376

merged 2 commits into from
Oct 25, 2018

Conversation

sveyret
Copy link
Contributor

@sveyret sveyret commented Oct 24, 2018

@oliviertassinari oliviertassinari changed the title Correct augmentColor definition for Typescript [theme] Correct augmentColor definition for Typescript Oct 24, 2018
@oliviertassinari oliviertassinari changed the title [theme] Correct augmentColor definition for Typescript [theme] Correct augmentColor TypeScript definition Oct 24, 2018
Copy link
Member

@eps1lon eps1lon left a comment

Choose a reason for hiding this comment

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

Bonus points if you add some tests for the declarations.

You can simply create a file named ´createPalette.spec.tsnext to the declarations and write some usage statements in there (e.g. examples from the unit tests) and annotate those statements with// $ExpectError` where you think ts should report at compile time.

e.g. packages/material-ui/TextField/TextField.spec.tsx is such a test for type declarations.

packages/material-ui/src/styles/createPalette.d.ts Outdated Show resolved Hide resolved
@oliviertassinari
Copy link
Member

💯 for adding new TypeScript and unit tests.

@sveyret
Copy link
Contributor Author

sveyret commented Oct 25, 2018

Actually, shade parameters are useless with a SimplePaletteColorOption which does not include color variations… So I made an override, at last, as @eps1lon suggested. I also added Typescript tests.

@sveyret
Copy link
Contributor Author

sveyret commented Oct 25, 2018

I have another suggestion. As you defined the type PartialColor, and as it seems this partial color should at least have a [500] entry, shouldn't we write:

export type ColorPartial = Partial<Color> & {500: string};

This would prevent from giving an empty object {} as a color.

Edit: Actually, this may not be a good idea, because if the color needs to have at least one entry, the one defined in main shade, it does not need to be 500.

@oliviertassinari oliviertassinari merged commit d5d1191 into mui:master Oct 25, 2018
@sveyret sveyret deleted the augmentColor branch October 26, 2018 05:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants