plouc / nivo

nivo provides a rich set of dataviz components, built on top of the awesome d3 and React libraries
https://nivo.rocks
MIT License
13.19k stars 1.03k forks source link

ResponsiveBar prop types don't align with documentation #1735

Closed Tbhesswebber closed 2 years ago

Tbhesswebber commented 3 years ago

Describe/explain the bug

When applying the label prop to ResponsiveBar Nivo will only allow functions that return strings, rather than functions that return SVG Elements.

To Reproduce

Steps to reproduce the behavior:

  1. Create a function that accepts BarDatum and returns an SVG element
  2. Pass it to <ResponsiveBar />

Expected behavior

TypeScript allows me to pass a function that returns an SVGAElement without me having to cast the return type with as unknown as string.

TypeScript TS 4.3.5

Additional context

This issue seems to stem from the fact that BarSvgProps extends BarCommonProps, which uses the Nivo core PropertyAccessor. The second type parameter is string, which is true for Canvas charts, but for SVG charts, it should also allow SVGAElement (PropertyAccessor<Datum, string | SVGAElement>)

plouc commented 3 years ago

@Tbhesswebber it returns a string because it can be used in various contexts: SVG, canvas, tooltip... If you want to customize the rendering of the label, you should rather use a custom bar component or an extra layer.

Tbhesswebber commented 3 years ago

@plouc - Can you clarify how those solutions would look? I don't see a way to do this without type casting even with your hints...

On a different note, I'm also confused why blocking this would be a preferable route over making the actual API surface align with the documented one

plouc commented 3 years ago

@Tbhesswebber, as the label component cannot be customized, supporting SGV elements could actually be an option at the moment, but I still think we should have dissociated computing the label, as a simple string, and having a custom rendering, being a custom SVG component or a function taking in the canvas 2d rendering context plus the label props, mixing both is a bit confusing, and not as portable.

I'm reopening it for type adjustments then.

wyze commented 3 years ago

I think we could extract out the label to a BarLabel component then support a labelComponent prop to override the default BarLabel. Just like we do with arcs and ArcLinkLabel.

Tbhesswebber commented 3 years ago

Is there a preference on type style? In order to be the least disruptive, I went the route of passing a second type parameter to BarCommonProps that is either the string SVG or a never. Tests and Linting both pass.

Is this good enough for the initial type-alignment fix?


    indexBy: PropertyAccessor<RawDatum, string>
    keys: string[]

    maxValue: 'auto' | number
    minValue: 'auto' | number

    margin?: Box
    innerPadding: number
    padding: number

    valueScale: ScaleSpec
    indexScale: ScaleBandSpec

    enableGridX: boolean
    gridXValues?: GridValues<string | number>
    enableGridY: boolean
    gridYValues?: GridValues<string | number>

    borderColor: InheritedColorConfig<ComputedBarDatumWithValue<RawDatum>>
    borderRadius: number
    borderWidth: number

    enableLabel: boolean
    label: PropertyAccessor<
        ComputedDatum<RawDatum>,
        SVGSupport extends 'SVG' ? SVGAElement | string : string
    >
    labelFormat: string | LabelFormatter
    labelSkipWidth: number
    labelSkipHeight: number
    labelTextColor: InheritedColorConfig<ComputedBarDatumWithValue<RawDatum>>

    isInteractive: boolean

    tooltip: React.FC<BarTooltipProps<RawDatum>>

    valueFormat?: ValueFormat<number>

    legendLabel?: PropertyAccessor<LegendLabelDatum<RawDatum>, string>
    tooltipLabel: PropertyAccessor<ComputedDatum<RawDatum>, string>

    groupMode: 'grouped' | 'stacked'
    layout: 'horizontal' | 'vertical'
    reverse: boolean

    colorBy: 'id' | 'indexValue'
    colors: OrdinalColorScaleConfig<ComputedDatum<RawDatum>>
    theme: Theme

    annotations: AnnotationMatcher<ComputedBarDatum<RawDatum>>[]
    legends: BarLegendProps[]

    renderWrapper?: boolean

    initialHiddenIds: string[]
}```
plouc commented 3 years ago

@Tbhesswebber, I think we can have the prop properly defined on BarSvgProps and BarCanvasProps.

stale[bot] commented 2 years ago

This issue has been automatically marked as stale. If this issue is still affecting you, please leave any comment (for example, "bump"), and we'll keep it open. We are sorry that we haven't been able to prioritize it yet. If you have any new additional information, please include it with your comment!

stale[bot] commented 2 years ago

Closing this issue after a prolonged period of inactivity. If this issue is still present in the latest release, please create a new issue with up-to-date information. Thank you!

Ayush11139 commented 2 years ago

Can someone tell me How to keep label values above bars in bargraph?

rahulm-optimus commented 9 months ago

Can someone just show an example how to do in ResponsiveBar component in React or JS.