-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
[charts] Expose <ChartType>Series
type for all chart types
#18805
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] Expose <ChartType>Series
type for all chart types
#18805
Conversation
Deploy preview: https://deploy-preview-18805--material-ui-x.netlify.app/ Bundle size reportTotal Size Change: 0B(0.00%) - Total Gzip Change: ▼-81B(0.00%) Show details for 100 more bundles (22 more not shown)@mui/x-charts parsed: 0B(0.00%) gzip: 0B(0.00%) |
CodSpeed Performance ReportMerging #18805 will not alter performanceComparing Summary
|
<ChartType>Series
type for all chart types
b5a419e
to
377b170
Compare
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.
Looks good. If you want to go further, I by searching SeriesType
in the docs/data
folder, I noticed some places where the new type could be used.
- https://github.com/mui/mui-x/blob/master/docs/data/charts/radar/DemoRadarSeriesHighlight.tsx#L24
- https://github.com/mui/mui-x/blob/master/docs/data/charts/scatter/ScatterCSSSelectors.tsx#L6
I left comments where I found what I assume are inconsistencies
{ pathname: '/x/api/charts/bar-series', title: 'BarSeries' }, | ||
{ pathname: '/x/api/charts/funnel-series', title: 'FunnelSeries' }, | ||
{ pathname: '/x/api/charts/heatmap-series', title: 'HeatmapSeries' }, | ||
{ pathname: '/x/api/charts/line-series', title: 'LineSeries' }, | ||
{ pathname: '/x/api/charts/pie-series', title: 'PieSeries' }, | ||
{ pathname: '/x/api/charts/radar-series', title: 'RadarSeries' }, | ||
{ pathname: '/x/api/charts/scatter-series', title: 'ScatterSeries' }, |
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.
You could add them in the pocs/public/_redirects
file such that links to previous page still work
/x/api/charts/scatter-series-type/ /x/api/charts/scatter-series/ 301
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.
As I was adding these, I realized that maybe we want to keep the old pages?
The SeriesType
is required for the ChartDataProvider
as that component requires the type
to be specified.
Should we keep both?
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.
It feels a bit weird to have two API pages just because one property can be optional
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.
Yeah, true. I'll keep it as is, then
Part of #16802.
Expose
LineSeries
,BarSeries
, etc, so that it's easy to do something like:Also replaced the old
BarSeriesType
pages withBarSeries
as the latter are the ones that are exposed to the user.Added some missing docs as well.