feat(bullet): the tooltip shows up around the drawn part of the chart only #1278
Conversation
monfera
added
:tooltip
| @@ -61,8 +77,8 @@ export class Arc implements Mark { | |||
| protected readonly x: number; | |||
| protected readonly y: number; | |||
| protected readonly radius: number; | |||
| protected readonly startAngle: number; | |||
| protected readonly endAngle: number; | |||
| protected readonly startAngle: Radian; | |||
| const x = e.clientX - box.left; | ||
| const y = e.clientY - box.top; | ||
| const capture = fullBoundingBox(this.ctx, this.props.geoms); | ||
| if (capture.x0 <= x && x <= capture.x1 && capture.y0 <= y && y <= capture.y1) { | ||
| return picker(x - chartCenter.x, y - chartCenter.y); | ||
| } |
There was a problem hiding this comment.
This will execute the fullBoundingBox call at 60FPS, recalculating everytime the same capture rectangle.
Can we just reuse the one coming from the getCaptureBoundingBox selector?
There was a problem hiding this comment.
Thanks, good catch, done in a54735c
| this.setCanvasTextState(ctx); | ||
| const box = ctx.measureText(this.text); |
There was a problem hiding this comment.
Can we reuse measureText that also brings in the font, size etc?
There was a problem hiding this comment.
I thought about it, but these bounding box methods are mirrors of the actual rendering code (this.setCanvasTextState(ctx) and precise input is fully shared), so it's already as close to font, size etc. as possible. The function measureText in src/common assumes text Box objects which are only used in most partition charts, due to the multirow layout, where its purpose is, adaptive font sizing and line breaks, so it's not a close fit currently.
It'd be a good idea to eventually migrate bullet graphs to that though: the user would specify the bounding box into which the bullet/goal titles must fit. It'd be a larger, API-involving task. Also, then the bounding box would be given, because that's what the user wants to fill.
If you meant CanvasTextBBoxCalculator, it's a heavier weight machinery that attaches/detaches to the DOM tree, stateful, allocates resources to dispose etc., it didn't seem necessary here. Also, the method in the PR is safe when introducing some other text property (say, we add fontStyle or some such), because the same code is called for text styling, while the CanvasTextBBoxCalculator needs explicit passing of properties one by one. I'm interested in learning about the circumstances where CanvasTextBBoxCalculator works but a simpler approach wouldn't.
| const angleFrom: Radian = Math.floor(this.startAngle / arcBoxSamplePitch) * arcBoxSamplePitch; | ||
| const angleTo: Radian = Math.ceil(this.endAngle / arcBoxSamplePitch) * arcBoxSamplePitch; | ||
|
|
||
| for (let angle: Radian = angleFrom; angle <= angleTo; angle += arcBoxSamplePitch) { |
There was a problem hiding this comment.
We need to be sure that angleFrom <= angleTo if not this will end up in a infinite loop
There was a problem hiding this comment.
The code bit has been updated for this issue
# [33.2.0](v33.1.0...v33.2.0) (2021-08-06) ### Bug Fixes * heatmap snap domain to interval ([#1253](#1253)) ([b439182](b439182)), closes [#1165](#1165) * hex colors to allow alpha channel ([#1274](#1274)) ([03b4f42](03b4f42)) ### Features * **bullet:** the tooltip shows up around the drawn part of the chart only ([#1278](#1278)) ([a96cbb4](a96cbb4)) * **legend:** multiline labels with maxLines option ([#1285](#1285)) ([e0eb096](e0eb096))
Summary
An overall bounding box of all graphical elements is computed, with some padding especially around text and potentially thin bounding box rectangle shapes for Fitts law compliance. The tooltip now only activates within this padded, rectangular capture zone. The bounding box is approximate, as the user has no exact notion of which pixel is still gauge/goal/bullet and which isn't.
Closes #1187
Details
Preparational PR, which was just refactoring: #1246
Implementation details: the bullet/gauge/goal is composited from three geoms:
Each of them gained a method for computing an overall bounding box around it. While it'd be possible to convert the chart to SVG or HTML and use DOM event capture, either way there would still be a need for at least an extra element in the DOM tree. Due to Fitts law and other considerations, drawn shapes are only a subset of where the tooltip must activate.
It's a draft PR because the individual methods need further tweaks, to solve text alignment and layout offset issues.There are other small type and code layout improvements resulting from drive-by-coding. Each commit is either functional, or explicitly one of these misc improvements (mostly).
Issues
Checklist