fraserxu / react-chartist

⚛ React component for Chartist.js
MIT License
528 stars 96 forks source link

Fixed TypeScript definitions #80

Open Benjamin-Dobell opened 5 years ago

Benjamin-Dobell commented 5 years ago
Benjamin-Dobell commented 5 years ago

The reason I say the listener definition isn't great is because its type is far too wide. However, chartist itself doesn't have definitions. I'm presently using:

declare module 'react-chartist' {
    // Should probably be in 'chartist'...

    export type DrawElementType = 'area' | 'bar' | 'line' | 'point'

    export type ChartistSVGElement = {
        animate: (o: Object) => void
    }

    export type ChartistSVGPath = {
        clone: () => ChartistSVGPath,
        stringify: () => string,
        scale: (x: number, y: number) => ChartistSVGPath,
        translate: (x: number, y: number) => ChartistSVGPath,
    }

    export type ChartistSVGRect = {
        x1: number,
        y1: number,
        x2: number,
        y2: number,
        width: () => number,
        height: () => number,
    }

    export type DrawOptions = {
        element: ChartistSVGElement,
        index: number,
        type: DrawElementType,
        path?: ChartistSVGPath,
        chartRect?: ChartistSVGRect,
    }

    // Should be 'react-chartist'

    // There are several events, but they're not even officially documented within chartist. We at least (partially) handle 'draw'.
    export type ChartistListenerOptions = {[key: string]: Function} & {
        draw?: (o: DrawOptions) => void
    }

    // ... type definitions as in this PR
}

However, that type information should be in DefinitelyTyped for chartist, not here. Additionally, that's only a very rough typing for the draw event listener, just to meet my needs - just reproducing here in case anyone is interested.

Actually, chartist DefinitelyTyped definitions are wrong anyway, they should be augmented like:

import 'chartist'
import {IBarChartClasses as BarChartClasses} from 'chartist'

declare module 'chartist' {
    export interface IBarChartClasses {
        gridBackground?: string
    }

    export interface IBarChartOptions {
        classNames?: BarChartClasses
    }
}

Unfortunately, don't have time to submit a DefinitelyTyped PR at the moment, if anyone else wants to then go for it.

fraserxu commented 5 years ago

Thanks @Benjamin-Dobell for the fix, do you mind also bump the version number in this pull request? Will merge and publish a version after.

cironunes commented 5 years ago

Sorry to be that guy, but are you planning to merge this? Can I help somehow?

Benjamin-Dobell commented 5 years ago

@cironunes The Chartist types themselves are actually super messed up: https://github.com/DefinitelyTyped/DefinitelyTyped/pull/36244

For example, there is no such thing as a candle graph, so the changes in this PR are off as well. I wanted to get the above merged first (before fixing these), but alas I just don't have the energy to fight poor process.