[charts] Allow multiple axes in the tooltip #17058
Conversation
alexfauquette
added
enhancement
|
Thanks for adding a type label to the PR! 👍 |
|
Deploy preview: https://deploy-preview-17058--material-ui-x.netlify.app/ Updated pages: |
CodSpeed Performance ReportMerging #17058 will not alter performanceComparing Summary
|
| export interface UseChartInteractionState { | ||
| interaction: { | ||
| /** | ||
| * The item currently interacting. | ||
| */ | ||
| item: null | ChartItemIdentifier<ChartSeriesType>; | ||
| /** | ||
| * The x- and y-axes currently interacting. | ||
| * The x/y SVG coordinate of the "main" pointer |
There was a problem hiding this comment.
What is the "main" pointer?
There was a problem hiding this comment.
I thaught about multip pointer interaction. Since we only store one point, it's the main one. How to handle this multi-point is an open quesiton
...rnals/plugins/featurePlugins/useChartCartesianAxis/useChartCartesianInteraction.selectors.ts
Show resolved
Hide resolved
|
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
github-actions
bot
added
the
PR: out-of-date
github-actions
bot
removed
the
PR: out-of-date
| /** | ||
| * If `true`, the hook returns an array with an object per active axis. | ||
| * @deprecated In future version, the `multipleAxes=true` will be the unique behavior | ||
| */ | ||
| multipleAxes?: boolean; |
There was a problem hiding this comment.
Instead of overloading useAxisTooltip would it make sense to create a useAxesTooltips instead and deprecate this useAxisTooltip?
I suppose it would make it easier to identify the deprecated code, as well as writing a codemod. Plus, the hook name would more closely follow what's happening: we're obtaining the tooltip content of all axes.
There was a problem hiding this comment.
I tried to do it without breaking changes, And then realised I had to modify the tooltip content.
Based on your comment, I created useAxesTooltips(params) = useAxisTooltips({...params, multipleAxies: true}) to avoid having similar logic in tow files. In next major we will be able to merge the two file together.
github-actions
bot
added
the
PR: out-of-date
|
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
…into update-tooltip-axis
github-actions
bot
removed
the
PR: out-of-date
| index === null || value === null ? null : { index, value }, | ||
| ); | ||
| /** | ||
| * Know if tooltip is active for y-axis |
There was a problem hiding this comment.
Is this comment accurate? It's preceding a selector called selectorChartsInteractionTooltipXAxes, which leads me to believe that something is off.
github-actions
bot
added
the
PR: out-of-date
|
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
This is the followup of #17039
The issue
The current behavior of Tooltip with
trigger="axis"is handcrafted. The main idea wasThis does not scale anymore with:
Proposed aproach
triggerTooltippaprameter.seriesConfig. For now bar and line set heirs x/y axies as triggering the tooltip according to their directiontriggerTooltip=trueand test if the current point position corresponds to a value.Needed refinement before merging
allowMultipleAxes. If this parameter is false or undefined we only return the first axis. Otherwise we return the array of axes.Changelog
Breaking change
The tooltip DOM structure is modified to improve accessibility.
If you relied on it to apply some style or run tests, you might be impacted by this modification.
The axis tooltip displays a table per axis with the axis value in a caption.
Cells containing the series label are now
thcells instead oftd.The item tooltip content is a simple
pcomponent with aspanfor series label and item value.