-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
[charts][POC] Charts #20361
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
base: master
Are you sure you want to change the base?
[charts][POC] Charts #20361
Conversation
JCQuintas
commented
Nov 17, 2025
- I have followed (at least) the PR section of the contributing guide.
| <PieChartBase.Root {...data}> | ||
| <PieChartBase.Surface> |
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'm not sure about calling them PieChartBase.Root and PieChartBase.Surface
Mostly because we would end up with a mix of line and bar when using composition
<LineChart.Root>
<LineChart.Surface>
<LineChart.LinePlot />
<BarChart.BarPlot />
</LineChart.Surface>
</LineChart.Root>Maybe something more like that
<CommonChart.Root>
<CommonChart.Surface>
<Cartesian.Grid />
<LineChart.LinePlot />
<BarChart.BarPlot />
<Cartesian.XAxis />
<Cartesian.YAxis />
</CommonChart.Surface>
</CommonChart.Root>it's maybe just because I'm not used to it, but passing everything to something called Root feel weird
| <PieChartBase.Root {...data}> | ||
| <PieChartBase.Surface> |
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 about naming every thing with PieChartBase
Mostly because the container and the surface can be shared between charts.
Having dedicated namings for common aspect feels more natural
<Common.Root>
<Common.Surface>
<Cartesian.Grid />
<LineChart.LinePlot />
<BarChart.BarPlot />
<Cartesian.XAxis />
<Cartesian.YAxis />
</Common.Surface>
</Common.Root>Maybe it's just me who gets too old, but Root does nto seems trivial to know if it's just a provider or a dom elements that does something
alexfauquette
left a comment
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 PoC. The style seems to work nice. Fro the darck/light theme I assume we can use CSS selectorsto override CSS vars. The only interogation I still have is the left-to-right / RTL display. But I've seen base-ui have a new utils for that
|
|
||
| This example demonstrates the proposed architecture where charts functionality is split into two packages: | ||
|
|
||
| - **@mui/x-charts-base**: Framework-agnostic core with inline styles and a custom theme system |
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 shouldn't have any theme system in the headless part, it should come from the styled layer
| This example demonstrates the proposed architecture where charts functionality is split into two packages: | ||
|
|
||
| - **@mui/x-charts-base**: Framework-agnostic core with inline styles and a custom theme system | ||
| - **@mui/x-charts-material**: Material UI integration layer using material components and theming |
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.
Alongside the Material UI version, it would be nice to build a Plain CSS demo of the same component to see if we are not by any chance leaking any Material UI concepts in the headless pat.
| height: 'var(--chart-surface-height, 100%)', | ||
| width: 'var(--chart-surface-width, 100%)', |
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 should just provide the CSS variabels values in the style tag this is how those should be consumed.
| display: 'flex', | ||
| position: 'relative', | ||
| flexDirection: 'column', | ||
| alignItems: 'center', | ||
| justifyContent: 'center', | ||
| overflow: 'hidden', |
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 should be in the styled layer.
| touchAction: 'pan-y', | ||
| userSelect: 'none', | ||
| gridArea: 'chart', |
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 these are functional styles that we should always have they should be directly set on the element via the style tag. If not, it's enough to have them in a demo.
| gridArea: 'chart', | ||
| }, | ||
|
|
||
| '& .ChartsSurface-root:focus': { |
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.
Everything from here to the bottom should go in the styled layer/demo. Also make sure that all these elements are accessible on the styled layer, e.g. either as a component or something the user has to render.
| '--PieChart-arc-stroke': 'black', | ||
| }), | ||
|
|
||
| '--FocusIndicator-color': 'var(--Primary-color)', |
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.
No theme leaks, e.g. non Material UI libraries may not even have this CSS var, or it could look differently.
|
|
||
| return ( | ||
| <ChartDataProvider<'pie', PieChartPluginSignatures> {...chartDataProviderProps}> | ||
| <FakeCss>{children}</FakeCss> |
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 mentioned, remove FakeCss, and use the code in the demo directly, then we will see if there are some things that are worth extracting here. You can also duplicate this in Material UI.
| ); | ||
| }); | ||
|
|
||
| // We could use `ChartsSurface` directly, but I think we could propose a different pattern, like if you want a single chart, then |
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 would be much more clear if we rename these files to contain the part name, and split the different components into different files.
| return ( | ||
| <g {...other}> | ||
| {transformedData.map((item, index) => ( | ||
| <PieArc |
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 should discuss how people would be able to customize these elements, as well as the focus indicator element. Should this be maybe a children callback that useres can use to render directly their own PieArc elements?
| @@ -0,0 +1,49 @@ | |||
| import { styled } from '@mui/material'; | |||
|
|
|||
| export const FakeCss = styled('div')(({ theme }) => ({ | |||
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.
Is this the only CSS we need? If yes, it's great! :)
| @@ -0,0 +1,9 @@ | |||
| import { styled } from '@mui/material'; | |||
|
|
|||
| export const FakeCss = styled('div')(({ theme }) => ({ | |||
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.
Can't we just wrap the PieChart.Root component and provide all the styles here? Why FakeCSS? This is Material UI package, no?