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

Fix/3061 #3446

Merged
merged 3 commits into from
Oct 9, 2016
Merged

Fix/3061 #3446

merged 3 commits into from
Oct 9, 2016

Conversation

etimberg
Copy link
Member

@etimberg etimberg commented Oct 8, 2016

Fixes the weird caret position hover issue described in #3061

I took the opportunity to refactor the private methods and make them correctly private. While I was in the alignment function, I properly used the tooltip alignment options if specified rather than using the auto options.

…art.Element.transition when the animation finishes to set `_view = _model` caused problems during update because we were using `helpers.extend` all over the place.

I changed to code so that we regenerate the model variable rather than continuously extending the old version. I also removed unnecessary tooltip reinitializations from the controller which should improve overall performance during interaction.
@simonbrunel
Copy link
Member

Nice work, we finally get this caret fixed, thanks :)

function getTooltipSize(tooltip, model) {
var ctx = tooltip._chart.ctx;

var size = {
Copy link
Member

Choose a reason for hiding this comment

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

While you are moving code around, I would remove the use of local object to do computations for this:

var height = model.yPadding * 2; // Tooltip Padding
var width = 0;
// ...
height += titleLineCount * titleFontSize;
// ...
return {
    height: height,
    width: width
};

That would help reducing the minified size (height can be minified to h whereas size.height would be s.height).

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure, will do

*/
function getBackgroundPoint(vm, size, alignment) {
// Background Position
var pt = {
Copy link
Member

Choose a reason for hiding this comment

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

Same here: var x = vm.x; var y = vm.y; ... return { x: x, y: y };

@simonbrunel simonbrunel added this to the Version 2.4 milestone Oct 9, 2016
@etimberg etimberg merged commit d21a853 into master Oct 9, 2016
@etimberg etimberg deleted the fix/3061 branch October 9, 2016 16:28
exwm pushed a commit to exwm/Chart.js that referenced this pull request Apr 30, 2021
Solve weird animation issues with the tooltip. The optimization in Chart.Element.transition when the animation finishes to set `_view = _model` caused problems during update because we were using `helpers.extend` all over the place.

I changed to code so that we regenerate the model variable rather than continuously extending the old version. I also removed unnecessary tooltip reinitializations from the controller which should improve overall performance during interaction.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants