-
-
Notifications
You must be signed in to change notification settings - Fork 1k
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
feat(legends): migrate package to typescript #1512
Conversation
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit ebfcc21:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ultimately I just added a default...
data: [],
padding: 0
to each of my legend definitions. This got things building again and worked for everything but my line charts which still render but no longer show any legends.
} | ||
|
||
export type LegendProps = { | ||
data: Datum[] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is requiring a Datum[] here right? I can't find anything in the Legends guide or in the storybooks demonstrating the use of this or explaining what it's for. This and the required padding
above both broke every one of my charts.
|
||
type CommonLegendProps = { | ||
direction: LegendDirection | ||
padding: number | Record<'top' | 'right' | 'bottom' | 'left', number> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similarly, I never used the padding prop before and can't find any form of documentation describing how/what it is used for. Should this really be required?
https://github.com/plouc/nivo/blob/master/packages/line/src/Line.js#L193 You have to exclude |
@wyze cool, I wasn't sure if you'd see this post-merge review :)
Yea, unfortunately that triggers a typescript error since it's required, and so removing it causes the build to fail compilation. |
Close #1221
Close #1321