-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
[charts] Extract a tooltip plugin from the interaction one #20591
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.
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
Conversation
|
Deploy preview: https://deploy-preview-20591--material-ui-x.netlify.app/ Updated pages: Bundle size report
|
CodSpeed Performance ReportMerging #20591 will not alter performanceComparing Summary
Footnotes |
| export const selectorChartsInteractionItem = createSelector( | ||
| selectInteraction, | ||
| (interaction) => interaction?.item ?? null, | ||
| ); | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Replaced by selectorChartsTooltipPointerItem
| export const selectorChartsInteractionItemIsDefined = createSelector( | ||
| selectorChartsInteractionItem, | ||
| (item) => item !== null, | ||
| ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Replaced by selectorChartsTooltipPointerItemIsDefined
| store.set('interaction', { ...store.state.interaction, item: null }); | ||
| store.update({ | ||
| interaction: { ...store.state.interaction, pointer: null }, | ||
| tooltip: { ...store.state.tooltip, item: null }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Having the tooltip update here avoid to modify too many lines. Migth need to remove it when introducing the control
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why though? If the goal is to split them, shouldn't we isolate them more?
Or maybe do it in a new PR if it needs changing a lot of files 😆
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because I realized that while reviewing my own PR :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did it by modifying the "closest point" plugin because it's able to set the tooltip item. But I did not modify the cartesian/polar axis plugins, because they do not set values to the item tooltip.
| store.set('interaction', { ...store.state.interaction, item: null }); | ||
| store.update({ | ||
| interaction: { ...store.state.interaction, pointer: null }, | ||
| tooltip: { ...store.state.tooltip, item: null }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why though? If the goal is to split them, shouldn't we isolate them more?
Or maybe do it in a new PR if it needs changing a lot of files 😆
| Object.keys(itemToRemove).some( | ||
| (key) => | ||
| itemToRemove[key as keyof typeof itemToRemove] !== prevItem[key as keyof typeof prevItem], | ||
| ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cant we use it like this?
| Object.keys(itemToRemove).some( | |
| (key) => | |
| itemToRemove[key as keyof typeof itemToRemove] !== prevItem[key as keyof typeof prevItem], | |
| ) | |
| Object.keys(itemToRemove).some( | |
| (key: keyof ChartItemIdentifier<ChartSeriesType>) => | |
| itemToRemove[key] !== prevItem[key], | |
| ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, but this work
(Object.keys(itemToRemove) as Array<keyof ChartItemIdentifier<ChartSeriesType>>).some(
(key) =>
itemToRemove[key] !== prevItem[key],
)|
|
||
| export interface UseChartTooltipInstance { | ||
| /** | ||
| * Setter for the item the user is interacting with. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we update these docs? "Interaction" and "interacting" may not make sense anymore
| * Setter for the item the user is interacting with. | ||
| * @param {ChartItemIdentifier} newItem The identifier of the item. | ||
| */ | ||
| setItemTooltip: (newItem: ChartItemIdentifier<ChartSeriesType>) => void; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can do this later, but wouldn't it make more sense to call this setTooltipItem?
| */ | ||
| setItemTooltip: (newItem: ChartItemIdentifier<ChartSeriesType>) => void; | ||
| /** | ||
| * Remove item interaction if the current if the provided item is still the one interacting. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The case where itemToRemove isn't provided is not documented
| (Object.keys(itemToRemove) as Array<keyof ChartItemIdentifier<ChartSeriesType>>).some( | ||
| (key) => itemToRemove[key] !== prevItem[key], | ||
| ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can't we use fastObjectShallowCompare here?
| * @param {InteractionUpdateSource} interaction The source of the last interaction update (pointer or keyboard) | ||
| * @returns {void} | ||
| */ | ||
| setLastUpdate: (interaction: InteractionUpdateSource) => void; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| setLastUpdate: (interaction: InteractionUpdateSource) => void; | |
| setLastUpdateSource: (interaction: InteractionUpdateSource) => void; |
| instance.removeItemTooltip?.(); | ||
| instance.clearHighlight?.(); | ||
| instance.removeItemTooltip?.(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you intend to call removeItemTooltip? Seems a bit weird
| export type UseChartInteractionSignature = ChartPluginSignature<{ | ||
| instance: UseChartInteractionInstance; | ||
| state: UseChartInteractionState; | ||
| optionalDependencies: [UseChartTooltipSignature]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why does this plugin depend on the tooltip plugin? I couldn't find any reference to the tooltip plugin from this one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice catch. It's because initially the cleanInteraction() was also resetting the tooltip item
This is a preliminary PR before the one to control tooltip item
The
useChartInteractionhandles at the same time the follwoing stateitem: defines the item tooltippointer: {x, y}: defines the current coordinate of the pointerlastUpdate: the interaction that triggered the last update (keyboard or pointer)In terms of usage:
item: only for the item tooltippointer: {x, y}: for both the axis tooltip, and the axis highlightlastUpdate: for both tooltip and highlightThe idea is to keep in interaction only the state that is shared between multiple concept (highlight and tooltip) and in the tooltip plugin put what is only related to the tooltip.
The PR is splitted by commits to simplify the review