mui / mui-x

MUI X: Build complex and data-rich applications using a growing list of advanced React components, like the Data Grid, Date and Time Pickers, Charts, and more!
https://mui.com/x/
4.55k stars 1.33k forks source link

[charts] Remove charts wrapper `div` #15511

Open JCQuintas opened 2 days ago

JCQuintas commented 2 days ago
mui-bot commented 2 days ago

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

Generated by :no_entry_sign: dangerJS against 377ef358dfc5186f01fab64fc24e1092ac361458

codspeed-hq[bot] commented 2 days ago

CodSpeed Performance Report

Merging #15511 will improve performances by 9.17%

Comparing JCQuintas:remove-wrapper-div (f6c3fc8) with master (9ebf586)

Summary

⚡ 1 improvements
✅ 5 untouched benchmarks

Benchmarks breakdown

Benchmark master JCQuintas:remove-wrapper-div Change
ScatterChartPro with big data amount 196.1 ms 179.7 ms +9.17%
JCQuintas commented 2 days ago

@alexfauquette check this out. It seems to be possible to fully remove the wrapper div around the charts. 🤔 There are only minimal changes to the argos visuals

Still need to fix the tooltips though 😆

alexfauquette commented 2 days ago

It seems to be possible to fully remove the wrapper div around the charts. 🤔

Why are you trying to remove it? I'm looking at the Recharts DOM structure and it seems quite similar our current structure

<div class="recharts-responsive-container"> // Takes full parent size
  <div class="recharts-wrapper"> // Full size and position relative
    <svg class="recharts-surface">...</svg> // Full size
    <div class="recharts-legend-wrapper">...</div> // Position absolute
  </div>
</div>

Do you have a particular use case in mind where it's needed?

JCQuintas commented 2 days ago

It seems to be possible to fully remove the wrapper div around the charts. 🤔

Why are you trying to remove it? I'm looking at the Recharts DOM structure and it seems quite similar our current structure

<div class="recharts-responsive-container"> // Takes full parent size
  <div class="recharts-wrapper"> // Full size and position relative
    <svg class="recharts-surface">...</svg> // Full size
    <div class="recharts-legend-wrapper">...</div> // Position absolute
  </div>
</div>

Do you have a particular use case in mind where it's needed?

Not exactly needed, but feels useless.

My goal is that the user will be able to do:

function MyComponent() {
  const a = useBarSeries();

  return (
    <div>
      {a?.seriesOrder.map((id) => {
        const series = a.series[id];
        return <div key={id}>{series.data.join('')}</div>;
      })}
    </div>
  );
}

const MyChart = () => {
  return (
    <div style={{ display: 'flex', flexDirection: 'column' }}>
      <ChartDataProvider ... >
        <ChartsSurface ...>{...}</ChartsSurface>
        <MyComponent />
      </ChartDataProvider>
    </div>
  );
});

And this will result in a very simple initial structure.

<div>     // provided by user
  <svg /> // chart
  <div /> // provided by user
</div>

// current

<div>       // provided by user
  <div>     // responsive container
    <svg /> // chart
  </div>    // provided by user
  <div />
</div>

There is only one layer the user needs to think about for classes and props. And if they want to put stuff outside of the chart that has to take the same size, they can handle it themselves.

alexfauquette commented 1 day ago

I was surprised that this work

<ChartDataProvider ... >
    <ChartsSurface ...>{...}</ChartsSurface>
    <MyComponent />
</ChartDataProvider>

But it's because I thaugh we measure the parent size but we are measuring the element itself

https://github.com/mui/mui-x/blob/4aca5f5d48f042d031c36796c10b2afda3c61921/packages/x-charts/src/context/SizeProvider/useChartContainerDimensions.ts#L25-L26

I'm ok with the idea. I would be interested to see a component. For example the BarChart with an HTML legend

JCQuintas commented 1 day ago

I would be interested to see a component. For example the BarChart with an HTML legend

Would something like this be good? https://codesandbox.io/p/sandbox/mui-mui-x-x-data-grid-forked-qvc5q3?file=%2Fsrc%2Fdemo.tsx%3A33%2C31

Is there anything specific you would like to see? 😆

alexfauquette commented 1 day ago

Would something like this be good?

Yes, it looks nice. I hope people will not try too crazy stuff with the div that wrapp the SVG and the legend together.

JCQuintas commented 1 day ago

Would something like this be good?

Yes, it looks nice. I hope people will not try too crazy stuff with the div that wrapp the SVG and the legend together.

What do you mean by "too crazy"?

JCQuintas commented 1 day ago

Arbitrary absolute positioning also works: https://codesandbox.io/p/sandbox/mui-mui-x-x-data-grid-forked-xy38kf?file=%2Fsrc%2Fdemo.tsx%3A22%2C44&workspaceId=836800c1-9711-491c-a89b-3ac24cbd8cd8

alexfauquette commented 1 day ago

What do you mean by "too crazy"?

It's just that devs will probably use that with all the display options possible. But that's not a real issue. With the two examples you made it should be ok :+1:

JCQuintas commented 1 day ago

Regarding docs any idea where it should stand? I suppose Composition, but should we divide the docs into two pages?

ChartContainer composition vs ChartDataProvider composition?

Or inside composition we have a section for ## Accessing data inside HTML? 🤔

JCQuintas commented 1 day ago

~Btw should we remove the unstable_ from the useSeries hooks?~

created the PR: https://github.com/mui/mui-x/pull/15545

alexfauquette commented 20 hours ago

Regarding docs any idea where it should stand? I suppose Composition, but should we divide the docs into two pages?

What about a third option that would be to start with a general introduction about ChartDataProvider, ChartSurface, ChartBarPlot

And then a subsection explaining that we provide a helpêr ChartCOntainer that group ChartDataProvider and ChartSurface which is adapted to most usecases