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

Chart.Controller deprecation #3839

Merged
merged 4 commits into from
Feb 10, 2017
Merged

Conversation

simonbrunel
Copy link
Member

Deprecates Chart.Controller (and the nested chart property) by gathering chart and chart.chart properties at the same level (chart.chart now being an alias to chart). chartInstance variables have been renamed to chart since there is no more distinction. A new test file has been added to keep track of deprecations (test/global.deprecations.tests.js).

Fixes #2481

Copy link
Member

@etimberg etimberg left a comment

Choose a reason for hiding this comment

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

These look good. Will sync them and test them out

@@ -30,12 +30,12 @@
backgroundColor: color(window.chartColors.red).alpha(0.2).rgbString(),
borderColor: window.chartColors.red,
data: [
randomScalingFactor(),
Copy link
Member

Choose a reason for hiding this comment

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

are these just whitespace changes?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, my editor is configured to remove trailing whitespace, which is actually an ESLint rule, but examples are not part of the lint process.

Copy link
Member

Choose a reason for hiding this comment

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

ah, ok. makes sense :)

});
});

describe('Version 2.1.5', function() {
Copy link
Member

Choose a reason for hiding this comment

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

oh awesome, I was going to suggest we add this. You're one step ahead of me 😄

Copy link
Member

@etimberg etimberg left a comment

Choose a reason for hiding this comment

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

I tested these changes out. I tried the samples for all different chart types and did not notice anything. I also tested the zoom and annotation plugins using their sample files and they still worked.

@etimberg etimberg added this to the Version 2.6 milestone Jan 28, 2017
@etimberg etimberg merged commit ba6e041 into chartjs:master Feb 10, 2017
@simonbrunel simonbrunel deleted the chart-namespace branch February 11, 2017 00:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants