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

Fixed TypeScript definitions #80

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Benjamin-Dobell
Copy link

  • Chartist instead of Chartit
  • responsiveOptions instead of responseOptions
  • listener (not the best definition, but chartist itself doesn't even document the events)
  • Support for Candle graphs
  • Correctly enforced props i.e. if the type is specified as Line then the options (if specified) must be ILineChartOptions.

@Benjamin-Dobell
Copy link
Author

Benjamin-Dobell commented May 16, 2019

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
Copy link
Owner

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
Copy link

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

@Benjamin-Dobell
Copy link
Author

Benjamin-Dobell commented Aug 13, 2019

@cironunes The Chartist types themselves are actually super messed up: DefinitelyTyped/DefinitelyTyped#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.

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

3 participants