feat: Charts v0.1 release #2
Conversation
Welcome to Codecov 🎉Once you merge this PR into your default branch, you're all set! Codecov will compare coverage reports and display results in all future pull requests. Thanks for integrating Codecov - We've got you covered ☂️ |
pan-kot
requested review from
Al-Dani,
ernst-dev,
gethinwebster,
jperals,
just-boris,
michaeldowseza and
orangevolon
orangevolon
left a comment
orangevolon
left a comment
There was a problem hiding this comment.
This is a monumental PR! Thanks @pan-kot for driving this, charts look amazing.
|
Two issues related to hover effects which I don't think are expected:
column-hover.mov
|
| const renderers = getTooltipContentOverrides?.({ point: tooltip.point, group: tooltip.group }); | ||
| const getTrack = placement === "target" ? api.getTargetTrack : api.getGroupTrack; | ||
| const orientation = tooltip.point?.series.chart.inverted ? "horizontal" : "vertical"; | ||
| const position = (() => { |
There was a problem hiding this comment.
Is it expected that while hovering between bars, the tooltip keeps being rendered but changes position?
tooltip-jump.mov
There was a problem hiding this comment.
Yes, that is actually deliberate (almost). I propose that we keep it, unless a better solution is found in the future.
I originally thought that the problem is related to the target rectangles that we use for tooltip placement, but that is not true - the rectangles do not have any margin around the series they are related to.
The problem comes from the mouseover logic we use for groups matching. When the cursor is between columns, it can be in position that does not cross either of the track targets, see:
Screen.Recording.2025-06-25.at.17.28.16.mov
When the cursor does not intersect with either of the rectangles, we can choose to hide the tooltip completely. However, that creates an annoying UX when the tooltip constantly disappears and reappears as the cursor is moved across the chart. To mitigate that, we show the tooltip that corresponds to the closest group, and that creates the effect from the screen recording.
Now, when the tooltip switches between point and group modes, it is not really expected to change the placement. That is actually a bug that originates in the tooltip component, that checks the chart orientation on the point state. That is wrong, because the point is not present when the tooltip mode is group. The bug is addressed here: a0fdcfb
| zooming: { type: "x" }, | ||
| events: { | ||
| selection(event) { | ||
| setZoom([event.xAxis[0].min, event.xAxis[0].max]); |
There was a problem hiding this comment.
When zooming, the tooltip can be rendered outside of the chart. Should our component handle this?
There was a problem hiding this comment.
I think that eventually it should, but it is not very clear atm. I would keep it the way it is for now.
11 participants
Description
Introduces new charts components, based on Highcharts v12.
How has this been tested?
Review checklist
The following items are to be evaluated by the author(s) and the reviewer(s).
Correctness
CONTRIBUTING.md.CONTRIBUTING.md.Security
checkSafeUrlfunction.Testing
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.