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

fix(core / swarmplot): Improve core and swarmplot typedefs #1151

Merged
merged 3 commits into from
Oct 15, 2020

Conversation

areese159
Copy link
Contributor

When implementing custom layers I noticed that the Pattern helpers were not being exported in the type def which caused us to have to use @ts-ignore in our code base. I also noticed when working with swarmplots the type def also appeared to be slightly off. I propose two changes in this PR, the first being to export the pattern helpers in the type definition of @nivo/core and the second to give layers a real definition instead of any[] in @nivo/swarmplot. If anything is not correct please let me know and I will be more than happy to fix it.

@areese159 areese159 changed the title Improve core and swarmplot typedefs fix(core / swarmplot): Improve core and swarmplot typedefs Oct 8, 2020
Copy link
Contributor

@wyze wyze left a comment

Choose a reason for hiding this comment

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

Looks great! Just want to tighten up the layer types a bit.

@@ -40,6 +56,8 @@ declare module '@nivo/swarmplot' {
event: React.MouseEvent<any>
) => void

export type Layers<Datum> = ((props: LayerProps<Datum>) => JSX.Element) | string
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we make this consistent with other packages like bar or line. Change from string to a union of possible layers.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Of course, ill get those changes pushed up tomorrow.

@areese159
Copy link
Contributor Author

I think I have addressed your feedback, let me know if you want anything else changed.

@areese159 areese159 force-pushed the improve-core-swarmplot-typedefs branch from 32bf729 to d698a83 Compare October 12, 2020 17:31
@areese159
Copy link
Contributor Author

Sorry, I made a typo, I've amended the commit and pushed the change. This should be good for rereview.

Copy link
Contributor

@wyze wyze left a comment

Choose a reason for hiding this comment

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

Looks good, thank you!

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.

None yet

2 participants