Add tooltip boxWidth and boxHeight options #6995
Conversation
There was a problem hiding this comment.
I haven't really gotten a chance to review or consider yet, but my first reaction is that this generally seems like it'd be okay, but I think we call this just "box" in the legend (instead of "colorBox"): https://www.chartjs.org/docs/latest/configuration/legend.html
bjones526
force-pushed
the
custom-tooltips
branch
from
3e1d38c to
2735681
Compare
src/plugins/plugin.tooltip.js
Outdated
| @@ -275,7 +278,9 @@ function getTooltipSize(tooltip) { | |||
| + options.titleMarginBottom; | |||
| } | |||
| if (combinedBodyLength) { | |||
| height += combinedBodyLength * bodyFontSize | |||
| // Body lines may include some extra height depending on boxHeight | |||
| const bodyLineHeight = bodyFontSize > boxHeight ? bodyFontSize : boxHeight; | |||
There was a problem hiding this comment.
const bodyLineHeight = Math.max(boxHeight, bodyFontSize)
src/plugins/plugin.tooltip.js
Outdated
| height += combinedBodyLength * bodyFontSize | ||
| // Body lines may include some extra height depending on boxHeight | ||
| const bodyLineHeight = bodyFontSize > boxHeight ? bodyFontSize : boxHeight; | ||
| height += combinedBodyLength * bodyLineHeight |
There was a problem hiding this comment.
With the change in #6984, this isn't correct when boxHeight > bodyFontSize. I think the correct version is
height += combinedBodyLength ? bodyLineHeight + ((combinedBodyLength - 1) * bodyFontSize) : 0;There was a problem hiding this comment.
@etimberg Makes sense, you don't want to allocate all that space for the colorBox on lines where the colorbox is no longer drawn. Do you happen to have the configuration you were using for the chart in your screenshot handy?
There was a problem hiding this comment.
Not off hand. I believe I used https://github.com/chartjs/Chart.js/blob/master/samples/charts/bar/vertical.html and changed the labels on https://github.com/chartjs/Chart.js/blob/master/samples/charts/bar/vertical.html#L30 to
['January', 'February', 'March', ['April', 'line 2'], 'May', 'June', 'July']
src/plugins/plugin.tooltip.js
Outdated
| @@ -753,14 +760,15 @@ class Tooltip extends Element { | |||
| drawBody(pt, ctx) { | |||
| const me = this; | |||
| const {body, options} = me; | |||
| const {bodyFontSize, bodySpacing, bodyAlign, displayColors} = options; | |||
| const {bodyFontSize, bodySpacing, bodyAlign, displayColors, boxHeight, boxWidth} = options; | |||
| const bodyLineHeight = bodyFontSize > boxHeight ? bodyFontSize : boxHeight; | |||
There was a problem hiding this comment.
Same comment here re. Math.max()
bjones526
force-pushed
the
custom-tooltips
branch
from
2735681 to
d1d484f
Compare
|
@etimberg I pushed a change to support multiline tooltips that should work in cases where some lines include the color box (with a custom height) and others don't. I did assume that
|
|
@bjones526 looking good. Would it be possible to align the second and third lines of text with the parent row as well? I think it'll look a lot better |
|
@bjones526 this PR will need to be rebased. Hopefully it won't be too much trouble! |
|
@bjones526 just checking in to see if you're still interested in pursuing this PR. thanks! |
bjones526
force-pushed
the
custom-tooltips
branch
from
d1d484f to
9c1cedb
Compare
bjones526
force-pushed
the
custom-tooltips
branch
from
9c1cedb to
cc10e6f
Compare
|
@benmccann thanks for the reminder - rebased and pushed since things had gotten stale. |
|
@etimberg For the alignment - the unaligned rows in the screenshot are the before and after callbacks, which don't have the same indentation as lines that include the colorboxes. I'm happy to open another issue for more alignment options though! |
|
Ah, ok @bjones526 thanks for the clarification |
benmccann
changed the title
Adds a new options to set colorBoxHeight and colorBoxWidth on tooltips.
Relates to #6654