Conversation
I believe it's a mistake to only delete the metaset if it has a valid controller; see f191f2f for where this behavior was introduced. This is a minimal fix for chartjs#9653; as discussed there, it may also be worth updating `updateHoverStyle`. As of chartjs#7030, `this._metasets` should always be defined, so checking whether it's undefined is no longer necessary.
|
Very well described issue and PR 👍 This makes sense. A regression test would be good, have you already tried creating one? |
etimberg
left a comment
etimberg
left a comment
There was a problem hiding this comment.
Change looks good. I agree with adding a test
|
@joshkel have you had a chance to look at adding a test? |
|
I apologize for the extreme delay in following up on this. I looked briefly at adding some tests. The existing metasets tests seem to exercise I can try and add a test covering the specific scenario from #9653 tomorrow. |
kurkle
left a comment
kurkle
left a comment
There was a problem hiding this comment.
I had never used toHaveSize, so looked it up.
It'd be good to have a test for the specific issue.
test/specs/core.controller.tests.js
Outdated
| }); | ||
| it('cleans up metasets when the chart is destroyed', function() { | ||
| this.chart.destroy(); | ||
| expect(this.chart._metasets.length).toHaveSize(0); |
There was a problem hiding this comment.
| expect(this.chart._metasets.length).toHaveSize(0); | |
| expect(this.chart._metasets).toHaveSize(0); |
toHaveSize(expected)
expect the actual size to be equal to the expected, using array-like length or object keys size.array = [1,2];
expect(array).toHaveSize(2);
There was a problem hiding this comment.
Thanks for catching this. After taking another look, .toEqual([]) seems more explicit than .toHaveSize(0), so I went with that. (I'm too used to Jest and TypeScript; if there's a better way to write these tests, please let me know.)
3 participants
I believe it's a mistake to only delete the metaset if it has a valid controller; see f191f2f for where this behavior was introduced.
As of #7030,
this._metasetsshould always be defined, so checking whether it's undefined is no longer necessary.This is a minimal fix for #9653; as discussed there, it may also be worth updating
updateHoverStyle.