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

Migrate to Chart.js v3.0.0-beta.7 #203

Merged
merged 27 commits into from
Dec 9, 2020

Conversation

santam85
Copy link
Contributor

New PR including all changes present in #181 and #202 (sorry, couldn't restore that branch)
Removed ES6 features in library code.

Closes #180

@santam85 santam85 changed the title Chartjs next Migrate to Chart.js v3.0.0-beta.6 Nov 12, 2020
@stockiNail
Copy link
Contributor

@simonbrunel it's official that CHART.JS 3 will drop IE11 support. See issue chartjs/Chart.js#8042 and PR chartjs/Chart.js#8009.
Therefore, even if we removed Object.assign, it can be used.

Copy link
Member

@simonbrunel simonbrunel left a comment

Choose a reason for hiding this comment

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

Thanks to everyone who contributed to that PR 👍

Before going further, we will need to get rid of everything not related to v3 (such as the dependencies update). You can either submit a PR against master if you would like it for that PR, or simply drop it. I will also need the docs / samples to be deployed so I can check that nothing is broken in case it's not covered by unit tests.

Edit: I will have a look on updating all dependencies in master.

gulpfile.js Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
samples/advanced/custom-labels.html Outdated Show resolved Hide resolved
samples/advanced/multiple-labels.html Outdated Show resolved Hide resolved
samples/utils.js Show resolved Hide resolved
types/index.d.ts Outdated Show resolved Hide resolved
types/index.d.ts Outdated Show resolved Hide resolved
types/index.d.ts Outdated Show resolved Hide resolved
types/test/exports.ts Outdated Show resolved Hide resolved
types/test/tsconfig.json Outdated Show resolved Hide resolved
@stockiNail
Copy link
Contributor

@simonbrunel about the update of labels, I have create the use case.
The issue sounds to be there if you use display: 'auto' that in previous version is not there.
First draw:

bug1

The "top" labels will be showed only if there is an update or hovering a dataset item.

bug2

Let me know if you think it's a bug in order to submit an issue.

@kurkle
Copy link
Member

kurkle commented Nov 16, 2020

Let me know if you think it's a bug in order to submit an issue.

@stockiNail It could be related to this: #203 (comment)

@stockiNail
Copy link
Contributor

Let me know if you think it's a bug in order to submit an issue.

@stockiNail It could be related to this: #203 (comment)

@kurkle apologize, I have written here to avoid to forget it, my memory is not so persistent. I'm gonna wait for the merge into the branch of the project (or into the master) and then I'll submit it.

@santam85 santam85 changed the title Migrate to Chart.js v3.0.0-beta.6 Migrate to Chart.js v3.0.0-beta.7 Dec 8, 2020
@santam85 santam85 requested a review from simonbrunel December 9, 2020 08:12
@simonbrunel simonbrunel added this to the Version 2.x milestone Dec 9, 2020
datasets: [
{
data: [],
Copy link
Member

@simonbrunel simonbrunel Dec 9, 2020

Choose a reason for hiding this comment

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

@etimberg @kurkle it looks like the TS definition of the core library enforces passing data.labels and data.datasets[i].data. I don't remember if the library needs them in case of an empty chart (I thought undefined values was correctly handled). And for labels, I'm wondering if all charts really need this input (e.g. scatter/bubble charts).

@simonbrunel
Copy link
Member

@santam85 Can you give me write permissions to your fork so I can push more commits to this PR?

@santam85
Copy link
Contributor Author

santam85 commented Dec 9, 2020

@santam85 Can you give me write permissions to your fork so I can push more commits to this PR?

You should have now, i enabled access to mantainers

@simonbrunel simonbrunel merged commit ade8c87 into chartjs:chartjs-next Dec 9, 2020
@simonbrunel simonbrunel deleted the chartjs-next branch December 9, 2020 20:01
@simonbrunel
Copy link
Member

@santam85 @sgratzl @stockiNail Thank you so much for all your hard work on migrating this plugin to Chart.js v3!

I still need to address a few things in master and then rebase the chartjs-next branch before releasing the first beta.

simonbrunel pushed a commit that referenced this pull request Dec 10, 2020
simonbrunel pushed a commit that referenced this pull request Dec 17, 2020
@mschlitz-trux
Copy link

@simonbrunel Is there a way to pull down the beta for testing from NPM for datalabels?

@simonbrunel
Copy link
Member

@mschlitz-trux I will as soon as the following points are fixed:

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.

Compatibility with Chart js 3.0
7 participants