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

New types and TypeScript test for Pie component #216

Closed
wants to merge 8 commits into from
Closed

New types and TypeScript test for Pie component #216

wants to merge 8 commits into from

Conversation

Psvensso
Copy link
Contributor

@Psvensso Psvensso commented Jun 1, 2018

Resolves #215

The original theme prop was copied from the Bar component's .d.ts, it also have the same problems and is not resolved by this PR. These common types (theme, motion legends etc.) should all actually be defined in the @nivo/core package and imported. So the next step would for sure be to create the @nive/core .d.ts and then components after that.

Again he motion props are copied from the bar defs but should be refined and moved to core package.

I crated the theme types from the default theme props in core, when moved/implmented in to core this should probably be reworked and make sure all props are there. But for pie this should be enough?

I also added a types folder and a test .tsx files. I think this is good practice, a types folder can contain more types files if needed for a more complex component, and even local tsconfig.json files or lint.
Even if the pie is simple its nice to keep it consistent.

@Psvensso Psvensso changed the title Resolves #215 New types and TypeScript test for Pie component Jun 1, 2018
@plouc
Copy link
Owner

plouc commented Jun 4, 2018

@Psvensso, thank you. I've improved the package and already adjusted the definitions, sorry for wasting your time and let me know if you encounter more problems.

@plouc plouc closed this Jun 4, 2018
@Psvensso
Copy link
Contributor Author

Psvensso commented Jun 5, 2018

@plouc No problems, just great to see this fixed!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Problems with Pie TypeScript definition
2 participants