Skip to content

Conversation

@Jareechang
Copy link
Contributor

As title states, added patch for Issue #3746 with test cases. Please review when you have a chance! @etimberg or @simonbrunel.

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.

Other than my 2 very minor comments, I think this is good to go.

Good job @Jareechang

// Remember Last Actives
changed = !helpers.arrayEquals(me._active, me._lastActive);

if (!changed) {
Copy link
Member

Choose a reason for hiding this comment

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

Could you add a comment here explaining why this exists? It might be a good idea to include a link to the relevant github issue as well

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ah good idea, that i'll add that in.

});

it('Should not update if active element has not changed', function() {
var chartInstance = window.acquireChart({
Copy link
Member

Choose a reason for hiding this comment

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

[minor] We are trying to standardize on chart rather than chartInstance.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sure! quick fix, no problem.

(chartjs#3746)

skip handleEvent (and update) if the element has not change

changed chartInstance to chart in test

added comment for code preventing tooltip update if there is not changes
@simonbrunel simonbrunel added this to the Version 2.6 milestone Feb 6, 2017
@etimberg etimberg merged commit 8f21718 into chartjs:master Feb 10, 2017
exwm pushed a commit to exwm/Chart.js that referenced this pull request Apr 30, 2021
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.

3 participants