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.

Sign up for GitHub

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

Jump to bottom

Avoid tooltip truncation in x axis if there is enough space #3998

Merged
merged 6 commits into from
Nov 11, 2017
Merged

Avoid tooltip truncation in x axis if there is enough space #3998

merged 6 commits into from
Nov 11, 2017

Conversation

kaidohallik
Copy link
Contributor

Truncation up to caretSize pixels could happen if label text produced tooltip element with size width:

  • left side tooltip: width < x and width > x - caretSize
  • right side tooltip: width < chartWidth - x and width > chartWidth - x - caretSize

Default caretSize = 5, so with default configuration truncation up to 5 pixels could happen.

Current problematic behaviour can be viewed in http://codepen.io/kaidohallik/pen/gmLQbN
'Blue exactly bad labelllll' is truncated.
After changing caretSize: 10 to caretSize: 0 no truncation, all is shown correctly.

It can fix someones problems in #1731
It's improvement of #1840

etimberg
etimberg approved these changes Mar 11, 2017
View reviewed changes
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.

@kaidohallik could you add a test case that shows how this changes the rendering?

@kaidohallik
Copy link
Contributor Author

@etimberg I started coding tests. In this process I think I found additional improvement to #1731 - if calculated tooltip.xAlign='center' and tooltip text is long then this is truncated though there is space to show this tooltip.
I updated example in http://codepen.io/kaidohallik/pen/gmLQbN there is now another truncated tooltip 'Red very very very very very very long label' - it's possible to show this without truncation.
What is preferred further activity with this PR?

  • First option: I try to add tests only for this caretSize issue and will do new PR for additional improvement
  • Second option: I add new commit to this PR which solves more problems related to v2 beta: tooltips are sometimes truncated #1731 and write tests for both commits at once

@etimberg
Copy link
Member

Let's do more commits here

@kaidohallik kaidohallik changed the title In tooltip x align calculation take into account caretSize [WIP] Avoid tooltip truncation in x axis if there is enough space Mar 12, 2017
@kaidohallik kaidohallik force-pushed the tooltip-x-align-caret-size branch from 8664072 to 996927c Compare March 12, 2017 23:53
@kaidohallik kaidohallik changed the title [WIP] Avoid tooltip truncation in x axis if there is enough space Avoid tooltip truncation in x axis if there is enough space Mar 14, 2017
@kaidohallik
Copy link
Contributor Author

Work finished with this PR.

etimberg
etimberg reviewed Mar 14, 2017
View reviewed changes
src/core/core.tooltip.js Outdated
} else {
model.opacity = 0;
}

model.xAlign = alignment.xAlign;
model.yAlign = alignment.yAlign;
model.x = backgroundPoint.x;
model.xCaret = backgroundPoint.xCaret;
Copy link
Member

Choose a reason for hiding this comment

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

Would it make sense / be useful to also have a way to override the yCaret position 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.

After reading this comment I started thinking that why not to use caretX, maybe can avoid this xCaret complexity in code.
And seems that this current code can be replaced with x2 = vm.caretX;.
So I made new commit where I removed xCaret and made change described above.

In y axis direction tooltips seem to be working ok and no need for changes.

It's quite a lot commits here already. Should I rebase?

@kaidohallik kaidohallik mentioned this pull request Apr 4, 2017
Closed
@qiluo
Copy link

qiluo commented Sep 4, 2017

hopefully this could be included in 2.7 milestone, thanks

@etimberg
Copy link
Member

etimberg commented Sep 4, 2017

@simonbrunel looks like we both approved this. Do you think we should merge it for 2.7?

@benmccann
Copy link
Contributor

@kaidohallik could you rebase this PR so that it can be merged?

@kaidohallik
In tooltip x align calculation take into account caretSize
5616e98 
Truncation up to caretSize pixels could happen if label text produced tooltip element with size width:
* left side tooltip: width < x and width > x - caretSize
* right side tooltip: width < chartWidth - x and width > chartWidth - x - caretSize
Default caretSize = 5, so with default configuration truncation up to 5 pixels could happen.
@kaidohallik
avoid tooltip truncation if possible
6c1ca9e 
use whole chart area for displaying tooltip
@kaidohallik
in xAlign calculation take into account caretPadding
d800d4a
@kaidohallik
add tests for tooltip truncation avoid logic
ef18a57
@kaidohallik
use caretX instead of xCaret
dd668a3
@kaidohallik kaidohallik force-pushed the tooltip-x-align-caret-size branch from d8697ed to dd668a3 Compare November 9, 2017 14:49
@kaidohallik
Copy link
Contributor Author

@benmccann rebase done

@etimberg etimberg added this to the Version 2.8 milestone Nov 9, 2017
@etimberg
Copy link
Member

etimberg commented Nov 9, 2017

Looks like the build is failing with some linter errors. @kaidohallik can you fix and then I'll merge this

@kaidohallik
Copy link
Contributor Author

@etimberg lint errors fixed

@etimberg etimberg merged commit 683e86e into chartjs:master Nov 11, 2017
@kaidohallik kaidohallik deleted the tooltip-x-align-caret-size branch November 13, 2017 06:01
yofreke pushed a commit to yofreke/Chart.js that referenced this pull request Dec 30, 2017
@kaidohallik @yofreke
Avoid tooltip truncation in x axis if there is enough space (chartjs#…
f8e9882 
…3998)

* In tooltip x align calculation take into account caretSize

Truncation up to caretSize pixels could happen if label text produced tooltip element with size width:
* left side tooltip: width < x and width > x - caretSize
* right side tooltip: width < chartWidth - x and width > chartWidth - x - caretSize
Default caretSize = 5, so with default configuration truncation up to 5 pixels could happen.

* avoid tooltip truncation if possible
use whole chart area for displaying tooltip

* in xAlign calculation take into account caretPadding

* add tests for tooltip truncation avoid logic

* use caretX instead of xCaret

* fix lint errors
@snyk-bot snyk-bot mentioned this pull request Mar 9, 2020
Closed
@snyk-bot snyk-bot mentioned this pull request Apr 1, 2020
Open
exwm pushed a commit to exwm/Chart.js that referenced this pull request Apr 30, 2021
@kaidohallik @etimberg
Avoid tooltip truncation in x axis if there is enough space (chartjs#…
088fa5b 
…3998)

* In tooltip x align calculation take into account caretSize

Truncation up to caretSize pixels could happen if label text produced tooltip element with size width:
* left side tooltip: width < x and width > x - caretSize
* right side tooltip: width < chartWidth - x and width > chartWidth - x - caretSize
Default caretSize = 5, so with default configuration truncation up to 5 pixels could happen.

* avoid tooltip truncation if possible
use whole chart area for displaying tooltip

* in xAlign calculation take into account caretPadding

* add tests for tooltip truncation avoid logic

* use caretX instead of xCaret

* fix lint errors
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@etimberg etimberg etimberg approved these changes

@simonbrunel simonbrunel simonbrunel approved these changes

Assignees
No one assigned
Labels
type: enhancement
Projects
None yet
Milestone
Version 2.7.2
Development

Successfully merging this pull request may close these issues.
5 participants
@kaidohallik @etimberg @qiluo @benmccann @simonbrunel