Skip to content
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

[charts] Provide hooks to create custom tooltip #14377

Merged
merged 21 commits into from
Sep 16, 2024

Conversation

alexfauquette
Copy link
Member

@alexfauquette alexfauquette commented Aug 28, 2024

I'm updating the Tooltip Composition section to introduce a new hok useItemTooltip that returns all the necessary data to create an item tooltip.

If the idea seems ok, I will create the useAxisTooltip the same way

The styling of those custom components is based on the future default tooltip design

@alexfauquette alexfauquette added the component: charts This is the name of the generic UI component, not the React module! label Aug 28, 2024
@mui-bot
Copy link

mui-bot commented Aug 28, 2024

Deploy preview: https://deploy-preview-14377--material-ui-x.netlify.app/

Updated pages:

Generated by 🚫 dangerJS against c25d7dc

Copy link

codspeed-hq bot commented Aug 29, 2024

CodSpeed Performance Report

Merging #14377 will not alter performance

Comparing alexfauquette:tooltip (c25d7dc) with master (7120da2)

Summary

✅ 3 untouched benchmarks

Copy link
Member

@JCQuintas JCQuintas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I really like the separate files for the demos 🥇

This seem to be shaping nicely, I've left a few small suggestions to improve though 😄

formattedValue: string | undefined;
}

export function useItemTooltip<T extends ChartSeriesType>(): null | UseItemTooltipReturnValue<T> {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we use this hook ourselves?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried it, but it would be a mess because the slots.itemContent is in the middle of this hook.

It expects to get the series and getColor whereas the hook directly provides the item value and color

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't get it, since we are not using any input props nor new contexts here, I would guess this could be used anywhere in the stack 🤔

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You have 3 nested components

  • ChartsTooltip
  • ChartsItemTooltipRender
  • DefaultChartsItemTooltipContent

The first one gets the item identifier from the context
The second one gets the relevant axes and series from the context
The last one computes the color, formatted label, and formatted value and render the content

The hook is made to replace all of them. So it returns the computed values (the color, the formatted label, ...)

But we currently allow users to pass a slot to replace DefaultChartsItemTooltipContent which expect rought data (the defaultized series and a getColor callback)

So to do that we would need to extend the hook returned values as follow:

useItemTooltip: () => {
   color: string;
   formattedLabel: string;
   formattedValue: string;
+   /**
+   * @deprecated The needed information from the series item are already available from other attributes.
+   * /
+  series: DefaultizedSeries
+   /**
+   * @deprecated The color of the item is already available with `color` attribute
+   * /
+  getColor: (dataIndex: number) => string
}

Such that we could do something like

function ChartsItemTooltip(){
  const data = useItemTooltip();

  const Content = slotProps.itemContent ?? DefaultChartsItemTooltip;
  return <Content {...data} />
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could have an internal with that signature and a public one which filters out the unnecessary values. Then when we simplify customisation we can move to using the public one.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What would be the interest? It sounds like an extra effort for something we are nearly sure to remove in the next major

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The main idea is that, if we use public apis internally, then it is easier to maintain and track implementation issues. But it is reasonable to do that in a future version 😅

docs/data/charts/tooltip/ItemTooltip.tsx Outdated Show resolved Hide resolved
const tooltipPosition: MousePosition = {
...mousePosition,
// Add the y-coordinate of the <svg/> to the to margin between the <svg/> and the drawing area
y: svgRef.current.getBoundingClientRect().top + drawingArea.top,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there any way we can improve on svgRef.current.getBoundingClientRect().top?

Maybe we could have a useDrawingArea().screenCoodinate.top?

Or a standalone hook, as getBoundingClientRect is costly. Eg: useDrawingAreaScreenCoordinate().top

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be a hook that would update a state by calling the getBoundingClientRect when receiving an event about

  • a resize of the SVG
  • a scroll of the window

Could be interesting but more in an followup PR

docs/data/charts/tooltip/ItemTooltipTopElement.js Outdated Show resolved Hide resolved
docs/data/charts/tooltip/ItemTooltipTopElement.js Outdated Show resolved Hide resolved
@alexfauquette alexfauquette marked this pull request as ready for review September 5, 2024 13:30
Copy link
Member

@JCQuintas JCQuintas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Implementation is really good, we just need to fix the issue with the regerssion tests trying to snapshot the helper component files 😅

@alexfauquette
Copy link
Member Author

we just need to fix the issue with the regerssion tests trying to snapshot the helper component files 😅

We could ignore all files from the tooltip page because those demo need interaction to be meaning full. But for a more robust solution I updated the script such that it does not try to take screenshot if there is no default export

Signed-off-by: Alexandre Fauquette <45398769+alexfauquette@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: charts This is the name of the generic UI component, not the React module!
Projects
None yet
3 participants