fix(radar): typing for custom functions #1089
Conversation
- Exported all custom elements. - Changed `GridLabelCustomFunction` to `CustomGridLabel` (for consistency). - Made `CustomGridLabel` and `CurstomDotSymbol` function components and typed their props according to docs. - Made `legends` optional. - Fixed typo `IndexByCustomFunction` instead of `IndexByCustomFunctiono`
| type IndexByCustomFunction<D = any> = (datum: D) => string | number | ||
| export type CustomGridLabel = React.FC<GridLabelProps> | ||
| export type CustomDotSymbol = React.FC<DotSymbolProps> | ||
| export type CustomDotLabel = (...args: any[]) => React.ReactNode |
There was a problem hiding this comment.
I think these (CustomDotLabel and CustomFormatter) should be strictly typed too, I never used them and I honestly don't have enough knowledge about them to type them.
There was a problem hiding this comment.
I agree, but I guess we can add this later, what you updated is already a good addition.
| @@ -55,7 +67,7 @@ declare module '@nivo/radar' { | |||
| isInteractive?: boolean | |||
| tooltipFormat?: string | CustomFormatter | |||
|
|
|||
| legends: LegendProps[] | |||
| legends?: LegendProps[] | |||
There was a problem hiding this comment.
I couldn't find anything in the docs about whether the legends are optional or not, but there are some examples in the storybook with no legends (https://nivo.rocks/storybook/?path=/story/radar--custom-label-component) and I am already using them without legends in the project I am working on, actually typescript threw me an error because of this one lol.
There was a problem hiding this comment.
Yes, they're not, and lacking doc :/
| } | ||
|
|
||
| type IndexByCustomFunction<D = any> = (datum: D) => string | number | ||
| export type CustomGridLabel = React.FC<GridLabelProps> |
There was a problem hiding this comment.
In the docs this must return an SVG element, I didn't want to strictly type that, maybe we can a have a discussion about this?
There was a problem hiding this comment.
As the implementation is SVG only, enforcing an SVG element would be good, but I think it's fine to use the generic ReactNode type.
|
@AlaaZorkane I remember looking through this PR, I guess I didn't do a formal review or comment. Would you mind resolving the conflicts and I can get this merged? |
| @@ -55,7 +67,7 @@ declare module '@nivo/radar' { | |||
| isInteractive?: boolean | |||
| tooltipFormat?: string | CustomFormatter | |||
|
|
|||
| legends: LegendProps[] | |||
| legends?: LegendProps[] | |||
There was a problem hiding this comment.
Yes, they're not, and lacking doc :/
| } | ||
|
|
||
| type IndexByCustomFunction<D = any> = (datum: D) => string | number | ||
| export type CustomGridLabel = React.FC<GridLabelProps> |
There was a problem hiding this comment.
As the implementation is SVG only, enforcing an SVG element would be good, but I think it's fine to use the generic ReactNode type.
| type IndexByCustomFunction<D = any> = (datum: D) => string | number | ||
| export type CustomGridLabel = React.FC<GridLabelProps> | ||
| export type CustomDotSymbol = React.FC<DotSymbolProps> | ||
| export type CustomDotLabel = (...args: any[]) => React.ReactNode |
There was a problem hiding this comment.
I agree, but I guess we can add this later, what you updated is already a good addition.
|
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 6f8fc90:
|
|
Thanks @AlaaZorkane! |
GridLabelCustomFunctiontoCustomGridLabel(for consistency).CustomGridLabelandCurstomDotSymbolfunction components and typed their props according to docs.legendsoptional.IndexByCustomFunctioninstead ofIndexByCustomFunctiono