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

The type for "draw" method of VisualElement is not correct for PointElement #9488

Closed
stockiNail opened this issue Jul 28, 2021 · 2 comments · Fixed by #9489
Closed

The type for "draw" method of VisualElement is not correct for PointElement #9488

stockiNail opened this issue Jul 28, 2021 · 2 comments · Fixed by #9489

Comments

@stockiNail
Copy link
Contributor

Expected Behavior

I'm using the GEO chartjs controller and I open an issue sgratzl/chartjs-chart-geo#83 because passing using version 3.5.0 the points are not shown.

I have checked what is changed between 3.4.1 and 3.5.0 and I see the draw method of point element needs (it's mandatory otherwise the point is not drawn) to get also the chartArea.

Possible Solution

Updates the types adding to chartArea as argument to draw method to VisualElement:

export interface VisualElement {
  draw(ctx: CanvasRenderingContext2D, area: ChartArea): void;
  inRange(mouseX: number, mouseY: number, useFinalPosition?: boolean): boolean;
  inXRange(mouseX: number, useFinalPosition?: boolean): boolean;
  inYRange(mouseY: number, useFinalPosition?: boolean): boolean;
  getCenterPoint(useFinalPosition?: boolean): { x: number; y: number };
  getRange?(axis: 'x' | 'y'): number;
}

Make sense?

Environment

  • Chart.js version: 3.5.0
  • Browser name and version: FF 90.0.2
@kurkle
Copy link
Member

kurkle commented Jul 28, 2021

This is actually a bug in _isPointInArea, it should return true when there is no area, and the area is optional.

@stockiNail
Copy link
Contributor Author

@kurkle thank you !! I have seen that only line and point elements are using area and I thought about a bug in the return statement of _isPointInArea, but not sure because it sounds mandatory.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants