[charts] Decouple `margin` and `axis-size` #16418

JCQuintas · 2025-01-31T17:46:52Z

Fixes #16387
Fixes #16383

mui-bot · 2025-01-31T17:50:20Z

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

Generated by 🚫 dangerJS against cd1b875

codspeed-hq · 2025-01-31T18:04:28Z

CodSpeed Performance Report

Merging #16418 will degrade performances by 58.76%

_{Comparing JCQuintas:axis-size-poc (cd1b875) with master (9b0bc3f)}

Summary

❌ 6 regressions

⚠️ Please fix the performance issues or acknowledge them on CodSpeed.

Benchmarks breakdown

	Benchmark	`BASE`	`HEAD`	Change
❌	`BarChart with big data amount`	552.1 ms	1,233.8 ms	-55.26%
❌	`BarChartPro with big data amount`	821 ms	1,827 ms	-55.06%
❌	`LineChart with big data amount`	317.6 ms	770 ms	-58.76%
❌	`LineChartPro with big data amount`	134.3 ms	290.4 ms	-53.74%
❌	`ScatterChart with big data amount`	378.3 ms	604.7 ms	-37.44%
❌	`ScatterChartPro with big data amount`	52.5 ms	82.2 ms	-36.07%

JCQuintas · 2025-02-03T09:55:57Z

@alexfauquette having the config in either the plugin or in the component cause a re-render which slows up the chart.

An alternative could be to get rid of left/right/top/bottomAxis properties, move all the config into the x/yAxis, which will then process everything before they render as far as I'm aware. 🤔

eg:

<LineChart
  xAxis={[
    {
      data: ['A', 'B', 'C', 'D'],
      scaleType: 'point',
      position: 'right',
    },
  ]}
  series={[{ data: [40, 30, 20, 10] }]}
  height={200}
/>

alexfauquette · 2025-02-03T14:36:17Z

An alternative could be to get rid of left/right/top/bottomAxis properties, move all the config into the x/yAxis, which will then process everything before they render as far as I'm aware. 🤔

Sounds reasonable. Wwe will need to find a hack to let the selectorChartDrawingArea accept to support this new notion of axis dimensions depending on the existence of the cartesian axis plugging or not.

You would still have the plugin in the following order "dimensions > series > axes" but the dimensions could have a selector that uses xAxis and yAxis config if available. The selectorChartDrawingArea would depend on two selectors selectorChartMargin and selectorChartAxisSpace.

If later we want to adapt the size according the real space an axis takes we will also need to take into account the zoom and the tick we render.

I assume the idea is then to have something like

xAxis={[
    {
      data: ['A', 'B', 'C', 'D'],
      scaleType: 'point',
      position: 'right',
      width: 95 // specify the space taken by the axis.
    },
  ]}

alexfauquette · 2025-02-03T15:07:56Z

By the way, the removal of leftAxis, ... can be done in a non breaking way.

We can deprecate the props and keep default the same

JCQuintas · 2025-02-05T17:01:03Z

@mui/xcharts what do you think about this API? I still have to make height/width count towards the margins, but it was trivial to allow multiple axis on a side in this implementation 😄

import * as React from 'react';
import { BarChart } from '@mui/x-charts/BarChart';

export default function BasicBars() {
  return (
    <BarChart
      xAxis={[
        { scaleType: 'band', data: ['group A', 'group B', 'group C'], position: 'top' },
        { scaleType: 'band', data: ['group 1', 'group 2', 'group 3'], position: 'top' }
      ]}
      series={[{ data: [4, 3, 5] }, { data: [1, 6, 3] }, { data: [2, 5, 6] }]}
      margin={60}
      width={500}
      height={300}
    />
  );
}

alexfauquette · 2025-02-05T20:51:01Z

I was effectively thinking of this issue #16387 about multiple axis as a nice example of what this new axis config could fix :)

I'm good with the API, and adding width and height as properties that get defaulted on x/y axes.
I would be against something like { position: 'top', size: { width: 50, height: 150 } } because for x-axis user will only care about the height, and with y-axis only about width.

bernardobelchior · 2025-02-06T08:41:52Z

packages/x-charts/src/ChartsAxis/ChartsAxis.tsx


 export interface ChartsAxisProps {
  /**
   * Indicate which axis to display the top of the charts.
   * Can be a string (the id of the axis) or an object `ChartsXAxisProps`.
   * @default null
+   * @deprecated Use `xAxis[].position="top"` instead.


We're suggesting using xAxis, but xAxis isn't a prop of this component. I'm a bit confused by this, so I suspect a user would be too. Where should I set the xAxis prop to maintain the behavior?

🤔 true it is not a prop of the ChartsAxis but it is of the XxxChart since it reuses the props.

In this use-case I would assume there is no "single best approach", since the prop is not on this component, but also not on a single external component.

What we can do is provide a url to the migration guide where we can explain with specific examples.

Yeah, I'm not sure what's the best way to proceed here. I suppose a URL to the migration guide is helpful, at least, so unless we can think of any better option, that seems good to me 👍

I'm not super concerned by this point. I guess most of the users are using directly the ChartsXAxis/ChartsYAxis and they all now where the xAxis props goes because with series they are the most used

github-actions · 2025-02-07T08:44:35Z

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

…e-poc

alexfauquette · 2025-02-07T14:40:43Z

packages/x-charts/src/constants/index.ts

+export const DEFAULT_AXIS_SIZE = 30;
+export const DEFAULT_AXIS_SIZES = {
+  top: DEFAULT_AXIS_SIZE,
+  bottom: DEFAULT_AXIS_SIZE,
+  left: DEFAULT_AXIS_SIZE,
+  right: DEFAULT_AXIS_SIZE,
 };


I'm dubious about this approach. For me either you have an axis. ANd then this axis can have a default width/height properties. But the axis size should not have another default value than 0.

I don't see why it should get 30px space whereas their is no axis displayed

True, I was copying the previous sizes, (margin = 50, now margin = 20, axis = 30) but it doesn't make sense indeed. 😆

Is there a way to know which axis is actually going to be displayed beforehand? 🤔

My current issue is that the "default" axis, is added to the "available axis array", and counted towards sizing, even when not used. Any declared, but not used axis would count towards the "axisSize" with the current logic.

In the axis Components we have these:

// Skip axis rendering if no data is available // - The domain is an empty array for band/point scales. // - The domains contains Infinity for continuous scales. if ((ordinalAxis && domain.length === 0) || (!ordinalAxis && domain.some(isInfinity))) { return null; }

So I assume we can only get the scale domain after processing the data, which means I'll probably have to move this to selectors I guess 😢

So I assume we can only get the scale domain after processing the data

I don't see the relation between having the scale and displaying the axis. Users could on purpose:

defines multiple scales but show only one of them because they need a mix of points and linear

defines one scale and show it multiple times (for example on top and bottom or left and right)

Is there a way to know which axis is actually going to be displayed beforehand? 🤔

For me, it's part of your PR. The axes that are going to be displayed are the ones with

position defined (left, right, top, or bottom)

the first one of the defined axes

A simplification could be to say the default axes got by default position set to left/bottom and then only rely on the position to know which one should be displayed.

alexfauquette · 2025-02-07T14:43:36Z

...x-charts/src/internals/plugins/featurePlugins/useChartCartesianAxis/useChartCartesianAxis.ts

+        return { ...acc, top: acc.top + (cur.height || 0) };
+      }
+      return { ...acc, bottom: acc.bottom + (cur.height || 0) };


What about moving the default here

Suggested change

return { ...acc, top: acc.top + (cur.height || 0) };

}

return { ...acc, bottom: acc.bottom + (cur.height || 0) };

return { ...acc, top: acc.top + (cur.height || DEFAULT_AXIS_HEIGHT) };

}

return { ...acc, bottom: acc.bottom + (cur.height || DEFAULT_AXIS_HEIGHT) };

…e-poc

JCQuintas · 2025-02-07T17:26:06Z

packages/x-charts/src/internals/plugins/corePlugins/useChartDimensions/useChartDimensions.ts

+// TODO: fix dep cycle, should we move selectors to their own folder/file? Eg:
+// useChartCartesianAxis/selectors/chartXAxis.ts
+import {
+  selectorChartXAxis,
+  selectorChartYAxis,
+} from '../../featurePlugins/useChartCartesianAxis/useChartCartesianAxis.selectors';


@mui/xcharts the current file setup is prone to dep cycles unrelated to the needed selectors. Should we move to more unitary files for selectors?

Having all selectors in the same file is usually useful because we can chain them.

You have a special. For which I would be in favor of having a dedicated file like useChartCartesianAxisLayout.selectors.ts or useChartCartesianAxisRendering.selectors.ts such that it's explicit that it contains selectors about how axes impacts the layout.

Thos selectors could compute the rendered ids and the size required.

JCQuintas · 2025-02-07T17:26:46Z

packages/x-charts/src/internals/plugins/corePlugins/useChartDimensions/useChartDimensions.ts

+      (acc, id) => {
+        const axis = xAxis.axis[id];
+
+        // TODO: move this to the selectorChartXAxis, axis.isUsed? axis.shouldRender?


@mui/xcharts what do you think about moving this logic as a permanent part of the axis definition? This can then be used in the x/y axis renderers

I thaugth it's the role of position?: 'left'|'right with something like

axis.isUsed = !axis.position===undefined

I thaugth it's the role of position?: 'left'|'right with something like

axis.isUsed = !axis.position===undefined

Not really, because of

// Skip axis rendering if no data is available // - The domain is an empty array for band/point scales. // - The domains contains Infinity for continuous scales. if ((ordinalAxis && domain.length === 0) || (!ordinalAxis && domain.some(isInfinity))) { return null; }

We could remove it, but it would only display a line, and we would have to fix some other issues with having bad values 🤔

Those are pathological cases where there is no data to display. The axis should exist and we should reserve some space for it when data gets loaded.

Here is an illustration where I modify the left margin when there is no data for the y-axis

https://codesandbox.io/p/sandbox/pensive-dust-g5jl44?file=%2Fsrc%2FDemo.tsx%3A15%2C51

Capture.video.du.2025-02-11.10-01-44.mp4

Got it, makes sense

…e-poc

alexfauquette · 2025-02-10T10:06:17Z

packages/x-charts/src/ChartsAxis/ChartsAxis.tsx

+  const completeBottomAxisProps = (
+    bottomAxes.length === 0 && !xAxis[xAxisIds[0]].position ? [xAxis[xAxisIds[0]]] : bottomAxes
+  ).map((axis) => ({ ...axis, ...mergeProps(axis, slots, slotProps) }));
+  const completeLeftAxisProps = (
+    leftAxes.length === 0 && !yAxis[yAxisIds[0]].position ? [yAxis[yAxisIds[0]]] : leftAxes
+  ).map((axis) => ({ ...axis, ...mergeProps(axis, slots, slotProps) }));


This could be simplified if you setup by default axis.position === 'bottom' for the first item in axis pluggin

alexfauquette · 2025-02-10T10:14:54Z

packages/x-charts/src/internals/plugins/corePlugins/useChartDimensions/useChartDimensions.ts

+// TODO: fix dep cycle, should we move selectors to their own folder/file? Eg:
+// useChartCartesianAxis/selectors/chartXAxis.ts
+import {
+  selectorChartXAxis,
+  selectorChartYAxis,
+} from '../../featurePlugins/useChartCartesianAxis/useChartCartesianAxis.selectors';


Having all selectors in the same file is usually useful because we can chain them.

You have a special. For which I would be in favor of having a dedicated file like useChartCartesianAxisLayout.selectors.ts or useChartCartesianAxisRendering.selectors.ts such that it's explicit that it contains selectors about how axes impacts the layout.

Thos selectors could compute the rendered ids and the size required.

alexfauquette · 2025-02-10T10:17:18Z

packages/x-charts/src/internals/plugins/corePlugins/useChartDimensions/useChartDimensions.ts

+      (acc, id) => {
+        const axis = xAxis.axis[id];
+
+        // TODO: move this to the selectorChartXAxis, axis.isUsed? axis.shouldRender?


I thaugth it's the role of position?: 'left'|'right with something like

axis.isUsed = !axis.position===undefined

github-actions · 2025-02-11T08:16:24Z

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

…e-poc

JCQuintas · 2025-02-11T16:45:10Z

@alexfauquette FYI this will still be a breaking change for users who are using custom components. There is no way that the left/right/Axis props can be passed to the provider for correct calculation of sizes, so the "old way" would look odd, because it would not know that leftAxis was set, and that it should leave space for it.

Should we go full BC instead then? Would simplify some stuff, and we can try to provide some codemods

bernardobelchior · 2025-02-11T16:54:23Z

packages/x-charts/src/ChartsAxis/ChartsAxis.tsx


+// TODO: Add links to the migration docs for each prop


Should this be done in this PR or as a follow-up? Just making sure it doesn't fall through the cracks 😄

bernardobelchior · 2025-02-11T17:06:37Z

packages/x-charts/src/internals/defaultizeMargin.ts

+  input: LayoutConfig['margin'],
+  defaultMargin?: ChartMargin,
+): Partial<ChartMargin> | undefined;
+export function defaultizeMargin(


Although used across our codebase, "defaultize" doesn't seem to be used. I wonder if we should rename this to defaultMargin, withDefaultMargin or withFallbackMargin. defaultMargin would make more sense, but "default" it is both a verb and a noun, so it could be confusing.

Not a blocker, just something to think about.

Don't review too much this PR, as it is still in Draft 😆

packages/x-charts/src/internals/plugins/corePlugins/useChartDimensions/useChartDimensions.ts

JCQuintas added breaking change enhancement This is not a bug, nor a new feature component: charts This is the name of the generic UI component, not the React module! labels Jan 31, 2025

JCQuintas self-assigned this Jan 31, 2025

JCQuintas requested a review from alexfauquette January 31, 2025 17:46

JCQuintas force-pushed the axis-size-poc branch from 86b3461 to 12e239f Compare January 31, 2025 19:20

JCQuintas added 2 commits February 5, 2025 15:58

general improvements

644cd1f

multiple axis + axis offset + axis size

7ee2fe4

JCQuintas force-pushed the axis-size-poc branch from 12e239f to 7ee2fe4 Compare February 5, 2025 16:51

bernardobelchior reviewed Feb 6, 2025

View reviewed changes

JCQuintas added 7 commits February 6, 2025 13:05

Merge branch 'master' into axis-size-poc

977dd34

todo

f48e90b

calculate margins + axis size

caab7bb

defaultize margin fn

1b961f7

remove "calculatelegendmargin" behavior

74f320f

fix missing filechange

45d9e94

try document

a73e755

github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Feb 7, 2025

Merge commit 'd8f269155f3ca08f72cb3f40a142005db0e1f6e1' into axis-siz…

acc67bd

…e-poc

github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Feb 7, 2025

JCQuintas added 2 commits February 7, 2025 13:21

use params not state

0747b9e

add comment

ec16952

alexfauquette reviewed Feb 7, 2025

View reviewed changes

JCQuintas added 5 commits February 7, 2025 16:05

types

6424b51

Merge commit '798b73c92c614b042514050c85edbdfccfce0584' into axis-siz…

5bd7329

…e-poc

Revert "[charts] Document plugins for internal use"

1cf7de6

empty sides

75916db

render based on state rather than props

f1eb5dd

JCQuintas commented Feb 7, 2025

View reviewed changes

Merge commit '587f7c5757e0d66222ea1f3d37c923a0814e9d86' into axis-siz…

0daad2b

…e-poc

alexfauquette reviewed Feb 10, 2025

View reviewed changes

github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Feb 11, 2025

JCQuintas added 2 commits February 11, 2025 13:28

Merge commit 'e71e83180666ae867c95b2dccad6653619aa7e2c' into axis-siz…

f1c99ba

…e-poc

fix cycle

e21062d

Merge axisprops

ec5f0a7

bernardobelchior reviewed Feb 11, 2025

View reviewed changes

JCQuintas added 3 commits February 11, 2025 18:14

none

50d753c

fix pie charts

419a688

react to svg width/height changes

cd1b875

github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Feb 11, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[charts] Decouple `margin` and `axis-size` #16418

[charts] Decouple `margin` and `axis-size` #16418

JCQuintas commented Jan 31, 2025 •

edited

Loading

mui-bot commented Jan 31, 2025 •

edited

Loading

codspeed-hq bot commented Jan 31, 2025 •

edited

Loading

JCQuintas commented Feb 3, 2025 •

edited

Loading

alexfauquette commented Feb 3, 2025 •

edited

Loading

alexfauquette commented Feb 3, 2025

JCQuintas commented Feb 5, 2025

alexfauquette commented Feb 5, 2025

bernardobelchior Feb 6, 2025

JCQuintas Feb 6, 2025

bernardobelchior Feb 6, 2025

alexfauquette Feb 7, 2025

github-actions bot commented Feb 7, 2025

alexfauquette Feb 7, 2025

JCQuintas Feb 7, 2025

JCQuintas Feb 7, 2025

alexfauquette Feb 10, 2025

alexfauquette Feb 7, 2025

JCQuintas Feb 7, 2025

alexfauquette Feb 10, 2025

JCQuintas Feb 7, 2025

alexfauquette Feb 10, 2025

JCQuintas Feb 10, 2025

alexfauquette Feb 11, 2025

JCQuintas Feb 11, 2025

alexfauquette Feb 10, 2025

alexfauquette Feb 10, 2025

alexfauquette Feb 10, 2025

github-actions bot commented Feb 11, 2025

JCQuintas commented Feb 11, 2025

bernardobelchior Feb 11, 2025

bernardobelchior Feb 11, 2025 •

edited

Loading

JCQuintas Feb 11, 2025

[charts] Decouple margin and axis-size #16418

Are you sure you want to change the base?

[charts] Decouple margin and axis-size #16418

Conversation

JCQuintas commented Jan 31, 2025 • edited Loading

mui-bot commented Jan 31, 2025 • edited Loading

codspeed-hq bot commented Jan 31, 2025 • edited Loading

Merging #16418 will degrade performances by 58.76%

Summary

Benchmarks breakdown

JCQuintas commented Feb 3, 2025 • edited Loading

alexfauquette commented Feb 3, 2025 • edited Loading

alexfauquette commented Feb 3, 2025

JCQuintas commented Feb 5, 2025

alexfauquette commented Feb 5, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Feb 7, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Feb 11, 2025

JCQuintas commented Feb 11, 2025

Choose a reason for hiding this comment

bernardobelchior Feb 11, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

[charts] Decouple `margin` and `axis-size` #16418

[charts] Decouple `margin` and `axis-size` #16418

JCQuintas commented Jan 31, 2025 •

edited

Loading

mui-bot commented Jan 31, 2025 •

edited

Loading

codspeed-hq bot commented Jan 31, 2025 •

edited

Loading

JCQuintas commented Feb 3, 2025 •

edited

Loading

alexfauquette commented Feb 3, 2025 •

edited

Loading

bernardobelchior Feb 11, 2025 •

edited

Loading