Skip to content

Conversation

@prakhargupta1
Copy link
Member

@prakhargupta1 prakhargupta1 commented Nov 7, 2025

@prakhargupta1 prakhargupta1 added docs Improvements or additions to the documentation. scope: charts Changes related to the charts. labels Nov 7, 2025
@mui-bot
Copy link

mui-bot commented Nov 7, 2025

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

Updated pages:

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 0B(0.00%) 0B(0.00%)
@mui/x-charts-pro 0B(0.00%) 0B(0.00%)
@mui/x-charts-premium 0B(0.00%) 0B(0.00%)
@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 d71306d

@codspeed-hq
Copy link

codspeed-hq bot commented Nov 7, 2025

CodSpeed Performance Report

Merging #20239 will not alter performance

Comparing prakhargupta1:line-demo (d71306d) with master (6ef52fd)1

Summary

✅ 13 untouched

Footnotes

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

@prakhargupta1 prakhargupta1 added the type: enhancement It’s an improvement, but we can’t make up our mind whether it's a bug fix or a new feature. label Nov 7, 2025
@prakhargupta1 prakhargupta1 marked this pull request as ready for review November 7, 2025 07:20
@bernardobelchior
Copy link
Member

When zoom: brush and pan: drag are enabled and I drag using the slider the brush overlay appears. Also after zoom-in, another brush zoom is not possible as it starts to pan. Is this config not supposed to be set together?

There's an info here:

When modifying the zoom interaction configuration, care should be taken as to not create a bad user experience.
For example, the "drag" and "brush" interactions do not work well together.
If both are needed, the pointerMode and requiredKeys options described in the next sections can help.

@bernardobelchior
Copy link
Member

I think we're missing a legend because it isn't clear what the green and red lines mean.

Also, should we center the title and align the source to the left?

image

I wonder if we want to render the labels above the content. The COVID-19 label is barely visible

@prakhargupta1
Copy link
Member Author

When I had to add a Legend, I had to replace <ChartContainer> with <ChartDataProvider /> and <ChartsSurface />. It makes me think what's the point of a ChartContainer at the first place? When I need to combine charts through composition, won't it be a better DX to directly go to the lowest level?

Is there any particular reason why <ChartContainer> is better than:

<ChartDataProvider >
 <ChartsSurface > ...... </ ChartsSurface >
 </ChartDataProvider >

Also, shouldn't <ChartDataProvider > be <ChartsDataProvider >? For most of the components we have 'Charts' instead of Chart. There's only three components that have Chart in the component name: ChartDataProvider, ChartContainer and ChartZoomSlider.

@bernardobelchior
Copy link
Member

When I need to combine charts through composition, won't it be a better DX to directly go to the lowest level

Yeah, the container is there just to ease it a little bit, but maybe it doesn't provide enough DX improvement and it's a bit confusing.

Is there any particular reason why is better than

I guess the benefit is one less component and you don't need to know which props go to the data provider and which ones to the surface. If you look at the implementation, the container is basically only rendering the data provider and the surface.

Also, shouldn't be ? For most of the components we have 'Charts' instead of Chart. There's only three components that have Chart in the component name: ChartDataProvider, ChartContainer and ChartZoomSlider.

Yeah, I think we need to make this more homogenous. I'd be more in favor of removing the 's'. If you look at Base UI, the components are named Menu.Root, Menu.Trigger, not Menus.Root and Menus.Trigger.

@alexfauquette @JCQuintas what do you think? We could introduce the variant without the 's' now and start migrating.

@JCQuintas
Copy link
Member

Yeah, I think we need to make this more homogenous. I'd be more in favor of removing the 's'. If you look at Base UI, the components are named Menu.Root, Menu.Trigger, not Menus.Root and Menus.Trigger.

IIRC @alexfauquette told me the Charts is meant to be Charts Library, similar to Pickers.

I can't find the discussion anymore, but I argued that we should move to Chart instead.

IIRC this was after the fact that I had created both the <ChartContainer> and <ChartDataProvider />, but since those were released we couldn't change much 😆

@alexfauquette @JCQuintas what do you think? We could introduce the variant without the 's' now and start migrating.

Not sure, doesn't feel super clean. Might as well just do it in one go.

@bernardobelchior
Copy link
Member

Not sure, doesn't feel super clean. Might as well just do it in one go.

My point was to ease the migration rather than simplify our life. By supporting both in v8, users can migrate in the last version of v8 minimizing the number of changes they need to do in one go to upgrade to v9.

I did the migration from MUI v4 to v5 and it was painful because there were so many changes that it wasn't possible to review or split up PRs. I wouldn't want to cause that now that I'm on this side 😅

const xScale = useXScale();
const yScale = useYScale('unemployment-axis');
const unemploymentSeries = useLineSeries('unemployment');

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
const { instance } = useChartContext();

Copy link
Member

Choose a reason for hiding this comment

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

Not feasible for now since it's an internal hook

Comment on lines 136 to 143
if (
x == null ||
y == null ||
x < left ||
x > left + width ||
y < top ||
y > top + height
) {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if (
x == null ||
y == null ||
x < left ||
x > left + width ||
y < top ||
y > top + height
) {
if (!instance.isPointInside(x, y)) {

@alexfauquette
Copy link
Member

I removed the label for unemployement pick, and simplified the data to only keep months where we have both GDP and unemployement.

My idea is: This is a demonstration. We don't need to over complicate the data

@prakhargupta1 prakhargupta1 merged commit 0835caf into mui:master Nov 26, 2025
22 checks passed
@prakhargupta1 prakhargupta1 deleted the line-demo branch November 26, 2025 11:58
A-s-h-o-k pushed a commit to A-s-h-o-k/mui-x that referenced this pull request Dec 14, 2025
Co-authored-by: alex <alex.fauquette@gmail.com>
mapache-salvaje pushed a commit to mapache-salvaje/mui-x that referenced this pull request Dec 29, 2025
Co-authored-by: alex <alex.fauquette@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

docs Improvements or additions to the documentation. scope: charts Changes related to the charts. type: enhancement It’s an improvement, but we can’t make up our mind whether it's a bug fix or a new feature.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants