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

Enhance context acquisition on chart creation #3474

Merged
merged 1 commit into from
Oct 16, 2016

Conversation

simonbrunel
Copy link
Member

Add support for creating a chart from the canvas id and prevent exceptions, at construction time, when the given item doesn't provide a valid CanvasRenderingContext2D or when the getContext API is not accessible (e.g. undefined by add-ons to prevent fingerprinting). New jasmine matcher to verify chart validity.

Enhances #2807
Replaces #3303

me.controller = new Chart.Controller(context, config, me);

return me.controller;
var Chart = function(item, config) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if Chart.Controller can eventually just become the Chart variable. #2481 asked about this and now that everything is in the controller, we might be able to do it.

Only issue I see is with any dependencies that the controller needs but we need the Chart variable to be defined first.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree, Chart.Controller could become Chart (maybe Chart.DatasetController could also simply be Chart.Controller). I can investigate how to do that change and keep backward compatibility but that would be for another PR :)

message = 'Expected canvas to be an instance of HTMLCanvasElement';
} else if (!(chart.ctx instanceof CanvasRenderingContext2D)) {
message = 'Expected context to be an instance of CanvasRenderingContext2D';
} else if (typeof chart.height !== 'number') {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be useful to also check if height and width are NaN or is that outside the scope of this?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You absolutely right, typeof NaN === 'number', actually it should even be a finite number.

Add support for creating a chart from the canvas id and prevent exceptions, at construction time, when the given item doesn't provide a valid CanvasRenderingContext2D or when the getContext API is not accessible (e.g. undefined by add-ons to prevent fingerprinting). New jasmine matcher to verify chart validity.
@etimberg etimberg merged commit 4a5b5a0 into chartjs:master Oct 16, 2016
@simonbrunel simonbrunel added this to the Version 2.4 milestone Oct 16, 2016
@simonbrunel simonbrunel deleted the context-acquisition branch October 18, 2016 17:23
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.

2 participants