[charts] WIP: Bar range poc

[bernardobelchior]

• I have followed (at least) the PR section of the contributing guide.

[mui-bot]

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

Updated pages:

• docs/data/charts/bar-demo/bar-demo.md
• docs/data/charts/bars/bars.md

Bundle size report

Bundle	Parsed size	Gzip size
@mui/x-data-grid	0B(0.00%)	0B(0.00%)
@mui/x-data-grid-pro	0B(0.00%)	0B(0.00%)
@mui/x-data-grid-premium	0B(0.00%)	0B(0.00%)
@mui/x-charts	🔺+4.87KB(+1.44%)	🔺+1.22KB(+1.20%)
@mui/x-charts-pro	🔺+4.78KB(+1.09%)	🔺+1.13KB(+0.85%)
@mui/x-charts-premium	🔺+4.78KB(+1.09%)	🔺+1.11KB(+0.84%)
@mui/x-date-pickers	0B(0.00%)	0B(0.00%)
@mui/x-date-pickers-pro	0B(0.00%)	0B(0.00%)
@mui/x-tree-view	0B(0.00%)	0B(0.00%)
@mui/x-tree-view-pro	0B(0.00%)	0B(0.00%)

Details of bundle changes

Generated by 🚫 dangerJS against b48bf96

[github-actions]

This pull request has conflicts, please resolve those before we can evaluate the pull request.

[github-actions]

This pull request has conflicts, please resolve those before we can evaluate the pull request.

[bernardobelchior]

Another issue we'll have is that barLabel receives the value, whose type will change depending on the series type. However, barLabel is a prop of the BarChart, so it can't infer the types from the series.

I think we'll need to use the type augmentation that we tried in #20084.

Another option would be to move the barLabel to the series, where the series type is defined, and deprecate it on the chart.

[JCQuintas]

Another issue we'll have is that barLabel receives the value, whose type will change depending on the series type.

Can't we ask users to check type for now, and change the API on the next major?

export type BarItem = {
  seriesId: SeriesId;
  dataIndex: number;
} & (
  | {
      type?: 'bar';
      value: number | null;
    }
  | {
      type: 'bar-range';
      value: null;
      range: { x: number; y: number } | null;
    }
);

[bernardobelchior]

Can't we ask users to check type for now

What do you mean by this?

How wouldn't this be a breaking change? If I understood correctly, we consider type changes like this a breaking change

[JCQuintas]

Can't we ask users to check type for now

What do you mean by this?

How wouldn't this be a breaking change? If I understood correctly, we consider type changes like this a breaking change

This should add to the type, value stays with the same types, but if we are in a barrange we have the "range" prop instead of value, value:null in these cases.

// Current
const t1: BarItem = {
  seriesId: 'series-1',
  dataIndex: 0,
  value: 100,
};

// Would just become
const t2: BarItem = {
  seriesId: 'series-1',
  dataIndex: 0,
  value: 100,
  type: 'bar'
};

// BarRange
const t3: BarItem = {
  seriesId: 'series-1',
  dataIndex: 0,
  value: null,
  type: 'bar-range',
  range: {x:1,y:2}
};

[bernardobelchior]

Ah, you're right. I didn't understand it.

Yeah, that would work, I think, but it feels like we're taking tech debt and we'll need to clean up it later either way since I don't think that API is desirable.

If we opt for moving the barLabel to the series (or even incorporating that logic into valueFormatter) while maintaining the current barLabel, wouldn't that work? The current barLabel would only be called for bar series and its API would remain unchanged. We would also mark it as deprecated and nudge users to move to the series label formatter.

Do you see significant problems or cons with this approach?

[JCQuintas]

or even incorporating that logic into valueFormatter

🤔 true
We could even have more props in the SeriesValueFormatterContext if necessary. It sounds like a better approach indeed

[bernardobelchior]

🤔 true
We could even have more props in the SeriesValueFormatterContext if necessary. It sounds like a better approach indeed

Just realized that this wouldn't work without a breaking change either.

If we add this behavior to valueFormatter, any user who's currently setting a valueFormatter would starting having bar labels in their bar charts. They are off by default, so I think we need to move barLabel to the series instead. I don't think we can rely on valueFormatter without a breaking change.

[github-actions]

This pull request has conflicts, please resolve those before we can evaluate the pull request.

[bernardobelchior]

Move barLabel to series

[github-actions]

This pull request has conflicts, please resolve those before we can evaluate the pull request.

[codspeed-hq]

CodSpeed Performance Report

Merging #20033 will not alter performance

_{Comparing bernardobelchior:bar-range-chart-poc (b48bf96) with master (1938a4a)¹}

Summary

✅ 13 untouched

Footnotes

1. No successful run was found on master (6b304bf) during the generation of this report, so 1938a4a was used instead as the comparison base. There might be some changes unrelated to this pull request in this report. ↩

[bernardobelchior]

Requires #20161