-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
[charts-pro] Allow exporting a heatmap chart #17916
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
Thanks for adding a type label to the PR! 👍 |
Deploy preview: https://deploy-preview-17916--material-ui-x.netlify.app/ Updated pages: Bundle size reportTotal Size Change: Show 15 more bundle changes@mui/x-charts-pro parsed: |
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
2daa1de
to
e40d095
Compare
type PluginsPerSeriesType = { | ||
heatmap: HeatmapPluginsSignatures; | ||
line: LineChartProPluginsSignatures; | ||
scatter: ScatterChartProPluginsSignatures; | ||
bar: BarChartProPluginsSignatures; | ||
}; | ||
|
||
export type ChartProApi< | ||
TSeries extends ChartSeriesType = ChartSeriesType, | ||
TSignatures extends | ||
readonly ChartAnyPluginSignature[] = TSeries extends keyof PluginsPerSeriesType | ||
? PluginsPerSeriesType[TSeries] | ||
: AllPluginSignatures, | ||
> = NonNullable<NonNullable<ChartContainerProProps<TSeries, TSignatures>['apiRef']>['current']>; |
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 previous implementation would show setZoomData
as a valid API for the heatmap chart. This implementation isn't breaking (since it defaults to AllPluginSignatures
), but IMO we should encourage users to use ChartProApi<'scatter'>
or ChartProApi<'heatmap'>
for TypeScript to only suggest functions that are valid.
For composition, I think we should add a composition
"series type" that uses the DefaultPluginSignatures
. This way, when using composition, users should use ChartProApi<'composition'>
, and in the next major we can make the TSeries
parameter mandatory.
What do you think, @alexfauquette @JCQuintas?
* `ChartsRootSurface` is a wrapper around the `ChartsSurface` component that provides a reference to the chart root element. | ||
* This is only needed for charts that aren't wrapped in `ChartsWrapper`. | ||
*/ | ||
export const ChartsRootSurface = React.forwardRef<SVGSVGElement, ChartsRootSurfaceProps>( |
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.
Had to create this to set the chart root ref. An alternative would be to default to svgRef
in the useChartProExport
plugin, but I thought the current approach would be cleaner.
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.
This PR might solve your issue by introducing the wrapper to heatmap too
#17943
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.
Yes, it will. I can wait for that to be merged
CodSpeed Performance ReportMerging #17916 will not alter performanceComparing Summary
|
@@ -73,7 +73,7 @@ function ExportParamsSelector({ | |||
} | |||
|
|||
export default function ExportChartAsImage() { | |||
const apiRef = React.useRef<ChartProApi>(undefined); | |||
const apiRef = React.useRef<ChartProApi<'line'>>(undefined); |
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.
Unfortunately, I don't think we can keep the current API for ChartProApi
. Either we create a new type, or we need to accept this breaking change as a bug fix.
It is a bug because invalid types are accepted. For example, the heatmap's apiRef
doesn't contain a setZoomData
property, but the type says it does.
It's unlikely a lot of people are using it as it was released in v8.1.0, but it's technically a breaking change if we don't flag it as a bug.
Another option would be to create another type to avoid breaking users and deprecate this one. What are your thoughts?
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 could just go with the generic one but still allow users to narrow the types with the <'seriesType'>
notation.
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.
That doesn't work either. I suppose it's the same issue as trying to pass a RefObject<HTMLElement>
to a HTMLDivElement
.
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 don't get it, if the issue is that you are passing a wide type but it expects a narrow type, just make the expectation wide, so it can accept a narrow 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.
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
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
e29fac8
to
b4b0efe
Compare
b4b0efe
to
cf5411c
Compare
@@ -6,7 +6,7 @@ import { useChartContext } from '../context/ChartProvider'; | |||
* Get the ref for the root chart element. | |||
* @returns The root chart element ref. | |||
*/ | |||
export function useChartRootRef(): React.RefObject<HTMLDivElement | null> { | |||
export function useChartRootRef(): React.RefObject<HTMLElement | SVGElement | 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.
This can probably be reverted?
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.
Not sure to be honest. The reality is that any HTML or SVG elements should be accepted here.
However, I think doing that now would be a breaking change, so I'll revert it. Maybe that's something we want to change for v9?
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.
Oh, if we need to accept HTMLElement and SVGElement
IIRC we should use HTMLElement | (HTMLElement & SVGElement)
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.
HTMLElement | (HTMLElement & SVGElement)
? Why not HTMLElement | SVGElement
?
I'd like to make this change, but it's breaking and it's unfortunate that we need to ask users to cast the ref object to use it. I wonder if there's some way to infer the type 🤔
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.
Nvm I couldn't find the reference, I might have been mistaken then 🤔
I was thinking I used that somewhere to solve this issue of needing to cast the ref, but I tried it out and it doesn't work 😢
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.
If you need a broader definition you can also try to use Element
instead
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.
If you need a broader definition you can also try to use
Element
instead
Yeah, that could also work, but we still have the typing issue. I don't know how to fix it, but I haven't dug deeper.
packages/x-charts/src/internals/components/ChartsWrapper/ChartsWrapper.tsx
Outdated
Show resolved
Hide resolved
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 mapping from series type to their pluggin signature is elegant 👍
Part of #11746.
Also updated the print example to allow selecting different chart types.