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
Added Patch to update tooltip only when active element has changed #3856
Conversation
There was a problem hiding this 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
src/core/core.tooltip.js
Outdated
@@ -782,6 +782,11 @@ module.exports = function(Chart) { | |||
|
|||
// Remember Last Actives | |||
changed = !helpers.arrayEquals(me._active, me._lastActive); | |||
|
|||
if (!changed) { |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
test/core.tooltip.tests.js
Outdated
@@ -674,4 +674,65 @@ describe('Core.Tooltip', function() { | |||
expect(tooltip._view.dataPoints[0].x).toBeCloseToPixel(point._model.x); | |||
expect(tooltip._view.dataPoints[0].y).toBeCloseToPixel(point._model.y); | |||
}); | |||
|
|||
it('Should not update if active element has not changed', function() { | |||
var chartInstance = window.acquireChart({ |
There was a problem hiding this comment.
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
.
There was a problem hiding this comment.
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.
89db799
to
863afc5
Compare
(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
As title states, added patch for Issue #3746 with test cases. Please review when you have a chance! @etimberg or @simonbrunel.