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.

Sign up for GitHub

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

Jump to bottom

Migrate to Chart.js v3.0.0-beta.6 #202

Closed
wants to merge 15 commits into from
Closed

Migrate to Chart.js v3.0.0-beta.6 #202

wants to merge 15 commits into from

Conversation

santam85
Copy link
Contributor

@santam85 santam85 commented Nov 10, 2020 •
edited by simonbrunel
Loading

New PR including all changes present in #181
Here I fixed the last issues related to typings and cleaned the git history a bit.

Closes #180

@santam85
Copy link
Contributor Author

@simonbrunel finally got all test to pass. Not sure how to solve the netlify deployment preview as the docs app builds and works correctly locally for me. Would you be open to review, merge and release this under the next tag, so that I can continue development and release valor-software/ng2-charts#1276 for people to start testing?

kurkle
kurkle reviewed Nov 11, 2020
View reviewed changes
docs/guide/getting-started.md Outdated Show resolved Hide resolved
docs/guide/options.md Outdated Show resolved Hide resolved
samples/advanced/custom-labels.html Outdated Show resolved Hide resolved
samples/advanced/custom-labels.html Outdated
@@ -44,17 +44,20 @@

Samples.srand(0);

Chart.helpers.merge(Chart.defaults.global, {
Chart.helpers.merge(Chart.defaults, {
Copy link
Member

Choose a reason for hiding this comment

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

I'd use Chart.defaults.set('', { in these

Copy link
Member

Choose a reason for hiding this comment

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

I find the Chart.defaults.set('', {}) syntax a bit weird (the empty string). Could set() support one option instead (Chart.defaults.set({})) so if first argument is not a string, then apply passed options to the root.

samples/charts/doughnut.html Outdated Show resolved Hide resolved
src/positioners.js Outdated Show resolved Hide resolved
src/utils.js Outdated Show resolved Hide resolved
test/utils.js Outdated Show resolved Hide resolved
test/utils.js Outdated Show resolved Hide resolved
types/options.d.ts Outdated Show resolved Hide resolved
@sgratzl sgratzl mentioned this pull request Nov 11, 2020
Closed
@simonbrunel simonbrunel changed the base branch from master to chartjs-next November 11, 2020 20:29
@simonbrunel
Copy link
Member

@santam85 Thanks for this new PR. I will have a look at it but I can't promise it will be this week. It's also a lot to digest since I'm not familiar with v3 implementation, so a lot to catch-up.

An early feedback though: please revert changes in .eslintrc.yml and make sure that all features you use are compatible with IE11, which is, if I'm not wrong, still supported by Chart.js v3. This plugin doesn't rely on babel and I don't want to introduce it for a few es6 features. For example, you should not use object destructuring in src/label.js or Object.assign().

@stockiNail
Copy link
Contributor

An early feedback though: please revert changes in .eslintrc.yml and make sure that all features you use are compatible with IE11, which is, if I'm not wrong, still supported by Chart.js v3. This plugin doesn't rely on babel and I don't want to introduce it for a few es6 features. For example, you should not use object destructuring in src/label.js or Object.assign().

@santam85 I removed Object.assign() from src/label.js, as @simonbrunel suggested. Pushed a PR in your repo.

@simonbrunel I have some doubts about the IE11 support from chart.js beta 6. I submitted an issue (chartjs/Chart.js#8042) because it seems do not work on IE11 .

@santam85 santam85 closed this Nov 12, 2020
@santam85 santam85 deleted the branch chartjs:chartjs-next November 12, 2020 16:46
@santam85 santam85 deleted the chartjs-next branch November 12, 2020 16:46
@santam85
Copy link
Contributor Author

Sorry, deleted the branch by mistake 👎
Will reopen...

@santam85 santam85 mentioned this pull request Nov 12, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kurkle kurkle kurkle left review comments

@simonbrunel simonbrunel simonbrunel left review comments

Assignees
No one assigned
Labels
enhancement
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@santam85 @simonbrunel @stockiNail @kurkle @sgratzl