Turns off interpolation per default

[sebiniemann]

As discussed in #6598, this MR turns off the interpolation by default.

This follows the idea behind #5939 to avoid potentially misleading charts per default.

[benmccann]

This isn't being set in the right place. You can see by doing gulp build and then opening up the samples in the samples directory. They still have bezier curves on

[benmccann]

It looks like it's set here:

Chart.js/src/elements/element.line.js

Line 25 in cfb5fba

tension: 0.4

[etimberg]

I think there are some tests that need to be updated. Hopefully that's not too hard

[sebiniemann]

This isn't being set in the right place. You can see by doing gulp build and then opening up the samples in the samples directory. They still have bezier curves on

Thanks a lot for pointing this out 😃 I have just checked it and now its looking as intended.

I think there are some tests that need to be updated. Hopefully that's not too hard

Is this due to some pixel-based render testing? I am not really sure how to update this, and whether I can do it in a way for you to review it later on.

[benmccann]

Yeah, it looks like most (or maybe all) are due to pixel matching. If you run the tests locally with gulp test --watch it will show the failing tests with the expected and actual images. You can save the actual in place of the expected and the test will start passing. We'll see the image diff in the PR. I don't know if there's a better way to update than clicking "Save as..." one-at-a-time on each. Maybe you could code a mode that automatically overwrites images for failing tests or for all tests. Not sure if that's faster or slower than the one-at-a-time method :-)

[sebiniemann]

Not sure if this previously occurred. In order to run the tests, I also hat to add babel-eslint and eslint-plugin-import to the package.json. Otherwise, I would get some missing package errors.

[etimberg]

Test files look good to me. Only thing that stood out is test/fixtures/controller.line/fill/order-default.png where the old test appears to have part of the line bleed through the green fill.

A few notes:

• yarn.lock should be removed. If the new packages are needed, package-lock.json should be updated
• We should add a note about the default change the v3 migration docs

[sebiniemann]

Only thing that stood out is test/fixtures/controller.line/fill/order-default.png where the old test appears to have part of the line bleed through the green fill.

This also surprised me. Wasn't expecting this to be influenced by lineTension.

yarn.lock should be removed. If the new packages are needed, package-lock.json should be updated

Done

We should add a note about the default change the v3 migration docs

Is this also missing for beginAtZero?

[etimberg]

For that image I suspect the issue was likely a bad initial image (we have a small tolerance for the comparisons because sometimes things shift a bit based on browser implementation) so i wouldn't worry about it.

It might be missing for beginAtZero as well. If so, I would support adding it (perhaps a new heading in that document for default config changes)

[benmccann]

Thanks for this!

I would update the migration guide and elements config page

Also https://www.chartjs.org/docs/master/general/performance#automatic-data-decimation-during-draw says:

tension is 0, stepped is false (default) and borderDash is [] (default).`

You could change this now to state that these are all the default values. Also, it looks like maybe there's a stray backtick at the end of that sentence? Mind fixing while you're at it?

[etimberg]

Looks like this needs a rebase.

[etimberg]

@sebiniemann have you had a chance to look at the rebase for this? I'd love to get it in

[sebiniemann]

Not yet, will look into it over the weekend

[sebiniemann]

Should be fine now. Had chosen the wrong base branch.

[benmccann]

This PR looks good to me overall. I'm curious why you had to add this and the other estlint package. Did they catch any existing errors? Babel isn't used to build our main output file anymore for es6/npm users and is now only used for the UMD build. I'm guessing this isn't really needed since we've removed the experimental language features

[sebiniemann]

Yes, this isn't needed/existing anymore after merging with the most recent master

[kurkle]

We are developing in master (or 2.9), release branches are for release tooling.

The base branch was changed.

[etimberg]

I changed the base back to master since that's where dev code goes.

[sebiniemann]

@kurkle and @etimberg Thanks for the advice. It is now also merged with the most recent master (and any conflicts are resolved).

However, I am now getting a single test error on windows, which does not seem to be related to this PR (or at least line tension).

Please find below the actual screenshot

[etimberg]

@sebiniemann the issue is using the font. Fonts are rendered slightly differently per browser. It does look unrelated; I'll update that test accordingly

[etimberg]

Re the changes in package-lock.json are those just from running npm i?

[sebiniemann]

Yes. I was surprised that these appeared at all after merging with master and running npm install as well as npm install --package-lock-only.

I am also getting these changes when running npm i on the master branch.

[benmccann]

awesome! thanks you for this. happy to have you contributing to the project! 😄

[benmccann]

oh, one thing I just realized though. we should also add to docs/docs/getting-started/v3-migration.md a mention that the default value has changed

[benmccann]

@sebiniemann sorry, but would you be able to rebase this PR? I think if you do that and add a note to the migration guide that should be about it before we can merge this unless @kurkle has any additional thoughts

Thanks again for this PR!

[sebiniemann]

@etimberg

I am currently afk and only had enough time for the merge, resolving conflicts and new test screenshots.

If you want to merge soon, I can also open an new PR for the migration guide (and also adding a note about beginAtZero).

[benmccann]

I'd be fine with merging and doing the docs update in a follow up since I think it will be easy to have merge conflicts with this one since it touches so many tests

[etimberg]

Agreed, happy to see a later PR for the docs