Skip to content

Fix tooltip text alignment#4684

Closed
silverspectro wants to merge 6 commits intochartjs:masterfrom
silverspectro:fix-tooltip-text-alignment
Closed

Fix tooltip text alignment#4684
silverspectro wants to merge 6 commits intochartjs:masterfrom
silverspectro:fix-tooltip-text-alignment

Conversation

@silverspectro
Copy link

No description provided.

Copy link
Member

@simonbrunel simonbrunel left a comment

Choose a reason for hiding this comment

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

@silverspectro can you rebase this PR on master and remove changes related to your other PR #4638?

@silverspectro silverspectro force-pushed the fix-tooltip-text-alignment branch from 952f85f to c13a1c3 Compare August 21, 2017 17:47
@silverspectro
Copy link
Author

is this okay ?


// Titles
this.drawTitle(pt, vm, ctx, opacity);
this.drawTitle(pt, vm, ctx, opacity, tooltipSize);
Copy link
Member

Choose a reason for hiding this comment

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

Not sure this is the best solution. tooltipSize is just vm.width and vm.height (line 792). I would prefer just using vm.width and vm.height inside these functions since it avoids another parameter


// Draw Title, Body, and Footer
pt.x += vm.xPadding;
// pt.x += vm.xPadding;
Copy link
Member

Choose a reason for hiding this comment

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

[nit] please remove commented code

- Delete TooltipSize to use vm.height and vm.width
- Delete commented code
- Fix Typo
@benmccann
Copy link
Contributor

@silverspectro your changes are making the tests fail

ReferenceError: getBoxHeight is not defined

@benmccann
Copy link
Contributor

You may be interested in #4890 which is also defining getBoxHeight

@etimberg
Copy link
Member

Closing due to inactivity

@etimberg etimberg closed this Dec 31, 2017
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.

4 participants