[charts] Fix charts not passing className to root element #13647

JCQuintas · 2024-06-27T19:05:18Z

Fix conformance for all charts in 'mergeClassName', 'propsSpread', 'reactTestRenderer', 'rootClass', 'refForwarding'
Refs passed to ResponsiveChartContainer go to svg element.
Forward ref from Gauge and PieChart components
Prevent MediaQueryList error on jsdom

mui-bot · 2024-06-27T19:09:18Z

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

Generated by 🚫 dangerJS against f82dec6

alexfauquette

Refs passed to ResponsiveChartContainer go to wrapper div instead of svg element. Breaking? ⚠️

Yes, this seems to be breaking change. Even if it's not the standard, their is some consistency by always passing the ref to the SVG component.

packages/x-charts-pro/src/ResponsiveChartContainerPro/ResponsiveChartContainerPro.tsx

packages/x-charts/src/ChartContainer/ChartContainer.test.tsx

JCQuintas · 2024-07-03T09:13:49Z

Yes, this seems to be breaking change.

I don't remember if we decided to do V8 or not 😅, should we wait to merge this?

Even if it's not the standard, their is some consistency by always passing the ref to the SVG component.

I think if it is necessary to have it always on the SVG we could have a proper svgRef that can be passed in? 🤔 Conformance tests don't properly work without the ref there 😅

alexfauquette · 2024-07-03T11:38:18Z

I don't remember if we decided to do V8 or not 😅, should we wait to merge this?

The v8 should be in 6 months

I think if it is necessary to have it always on the SVG we could have a proper svgRef that can be passed in?

This would not fix the breaking change aspect. Someone using the ref to access the SVG will get access to a wrapping div after this modification

Conformance tests don't properly work without the ref there

Is it the main reason to do the modification?

JCQuintas · 2024-07-03T11:50:36Z

This would not fix the breaking change aspect. Someone using the ref to access the SVG will get access to a wrapping div after this modification

I didn't mean it as a solution, but rather as a feature. If we want the user to always have access to the SVG directly, we can have a prop that does exactly that. Otherwise I think the ref should probably go to the topmost element. Though I don't know how we handle it in the main lib.

My point of view is that if I have the topmost ref I can get any of the children elements I need, though I also see it being useful to have a ref directly to the SVG element.

…arts-charts-dont-pass-classname-to-root-element

JCQuintas · 2024-07-10T20:05:12Z

@alexfauquette reverted ref forwarding to forward to the SVG element.

alexfauquette

👍

oliviertassinari · 2024-07-15T10:34:36Z

packages/x-charts/src/BarChart/useBarChartProps.ts

@@ -50,6 +50,8 @@ export const useBarChartProps = (props: BarChartProps) => {
    onHighlightChange,
    borderRadius,
    barLabel,
+    className,
+    ...rest


The naming convention in the codebase?

Suggested change

...rest

...other

Added changes to #13844

oliviertassinari · 2024-07-15T10:41:45Z

packages/x-charts/src/ChartContainer/useChartContainerProps.ts

+import type { SeriesContextProviderProps } from '../context/SeriesContextProvider';
+import type { ZAxisContextProviderProps } from '../context/ZAxisContextProvider';
+import type { ChartContainerProps } from './ChartContainer';
+


Usually, we don't use blank lines in imports:

Suggested change

oliviertassinari · 2024-07-15T10:43:41Z

packages/x-charts/src/BarChart/BarChart.test.tsx

@@ -0,0 +1,30 @@
+import * as React from 'react';
+import { createRenderer, describeConformance } from '@mui/internal-test-utils';
+import { BarChart } from './BarChart';


It's a public API, it should be:

Suggested change

import { BarChart } from './BarChart';

import { BarChart } from '@mui/x-charts/BarChart';

to get a bit closer to an end-to-end test, and also make it clearer it's public.

x the other cases.

oliviertassinari · 2024-07-15T10:47:09Z

packages/x-charts/src/hooks/useReducedMotion.ts

+    // MatchMedia is not fully supported in all environments, so we need to check if it exists before calling addEventListener
+    mql.addEventListener?.('change', handleMediaChange);


Something doesn't make sense to me. If it's not fully supported in all environments, how is it possible for useMediaQuery() to rely on:

Suggested change

// MatchMedia is not fully supported in all environments, so we need to check if it exists before calling addEventListener

mql.addEventListener?.('change', handleMediaChange);

mql.addEventListener('change', handleMediaChange);

I conclude that one of the two is wrong (either in Core or in X)

Actually useColorScheme() uses mql.addListener, why not mql.addEventListener.

Both useMediaQuery.ts and useMediaQuery.test.js have ways of preventing the code from executing.

In the main code we seem to be testing for the presence of window.matchMedia, which we do in our code as well, but our tests add a stub for it in stubMatchMedia

On the useMediaQuery.test.js test themselves, it seems we use css-mediaquery to completely override the matchMedia behaviour implementing our own event emitter, which in turn never gets to the issue

Regarding using .addListener vs .addEventListener. .addListener is deprecated. Typescript marks it as deprecated and so does MDN

JCQuintas added 3 commits June 27, 2024 20:47

Update regular charts and containers

a7b8ddc

Try adding conformance test

c4fb2f1

Add basic test for classnames

eb2a2d8

JCQuintas added bug 🐛 Something doesn't work component: charts This is the name of the generic UI component, not the React module! labels Jun 27, 2024

JCQuintas self-assigned this Jun 27, 2024

JCQuintas added 10 commits July 2, 2024 19:31

Properly spread props to the correct elements

e7ef0ad

Add barchart conformance

c92a7ac

Add gauge conformance

740ee5d

Improve barchart

37befb8

Add linechart conformance

ae38aa1

Add piechart conformance

ce5a449

Add scatterchart conformance

6f736da

Add sparkline conformance

47c199c

Add width to tests

4333003

Add chartcontainer conformance

907f642

JCQuintas requested a review from alexfauquette July 2, 2024 18:19

JCQuintas added 2 commits July 2, 2024 20:21

Update api

50eb3f8

Fix wrong demo

0a9c09d

alexfauquette mentioned this pull request Jul 3, 2024

[code-infra] The parser of the describe conformance consider skip but not only config mui/material-ui#42832

Open

alexfauquette reviewed Jul 3, 2024

View reviewed changes

packages/x-charts-pro/src/ResponsiveChartContainerPro/ResponsiveChartContainerPro.tsx Outdated Show resolved Hide resolved

packages/x-charts/src/ChartContainer/ChartContainer.test.tsx Outdated Show resolved Hide resolved

JCQuintas added 3 commits July 3, 2024 11:07

move owner state in

97f9c1a

Use skip instead of only in conformance

037162a

fix api

067f8f7

JCQuintas added 2 commits July 3, 2024 17:38

Merge commit '9f19b9fc35b1a78ff09dcffc76eb30bd0b5cf1fb' into 13292-ch…

a6d39fe

…arts-charts-dont-pass-classname-to-root-element

Add use charts container props

6159258

JCQuintas added 3 commits July 10, 2024 21:31

Merge commit '38def519c262735ca32f31db1cc723d6f46c657e' into 13292-ch…

eb9c5bb

…arts-charts-dont-pass-classname-to-root-element

Unify responsive props

d4ea7e4

Revert ref to be in SVG

365528f

JCQuintas added 2 commits July 10, 2024 22:18

Fix gauge

9805508

Actually test refForwarding with correct types

f82dec6

JCQuintas requested a review from alexfauquette July 11, 2024 08:23

alexfauquette approved these changes Jul 11, 2024

View reviewed changes

JCQuintas merged commit 55aee56 into mui:master Jul 11, 2024
17 checks passed

JCQuintas deleted the 13292-charts-charts-dont-pass-classname-to-root-element branch July 11, 2024 14:13

oliviertassinari reviewed Jul 15, 2024

View reviewed changes

DungTiger pushed a commit to DungTiger/mui-x that referenced this pull request Jul 23, 2024

[charts] Fix charts not passing className to root element (mui#13647)

fd1e65a

alhabib5565 mentioned this pull request Aug 19, 2024

[Snyk] Upgrade @mui/x-data-grid from 7.3.0 to 7.11.1 alhabib5565/quick-shop-client#11

Open

thomasmoon pushed a commit to thomasmoon/mui-x that referenced this pull request Sep 9, 2024

[charts] Fix charts not passing className to root element (mui#13647)

a2e3423

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[charts] Fix charts not passing className to root element #13647

[charts] Fix charts not passing className to root element #13647

JCQuintas commented Jun 27, 2024 •

edited

Loading

mui-bot commented Jun 27, 2024 •

edited

Loading

alexfauquette left a comment

JCQuintas commented Jul 3, 2024 •

edited

Loading

alexfauquette commented Jul 3, 2024

JCQuintas commented Jul 3, 2024

JCQuintas commented Jul 10, 2024 •

edited

Loading

alexfauquette left a comment

oliviertassinari Jul 15, 2024

JCQuintas Jul 15, 2024

oliviertassinari Jul 15, 2024

oliviertassinari Jul 15, 2024

oliviertassinari Jul 15, 2024 •

edited

Loading

JCQuintas Jul 15, 2024

	import { BarChart } from './BarChart';
	import { BarChart } from '@mui/x-charts/BarChart';

		// MatchMedia is not fully supported in all environments, so we need to check if it exists before calling addEventListener
		mql.addEventListener?.('change', handleMediaChange);

	// MatchMedia is not fully supported in all environments, so we need to check if it exists before calling addEventListener
	mql.addEventListener?.('change', handleMediaChange);
	mql.addEventListener('change', handleMediaChange);

[charts] Fix charts not passing className to root element #13647

[charts] Fix charts not passing className to root element #13647

Conversation

JCQuintas commented Jun 27, 2024 • edited Loading

mui-bot commented Jun 27, 2024 • edited Loading

alexfauquette left a comment

Choose a reason for hiding this comment

JCQuintas commented Jul 3, 2024 • edited Loading

alexfauquette commented Jul 3, 2024

JCQuintas commented Jul 3, 2024

JCQuintas commented Jul 10, 2024 • edited Loading

alexfauquette left a comment

Choose a reason for hiding this comment

oliviertassinari Jul 15, 2024

Choose a reason for hiding this comment

JCQuintas Jul 15, 2024

Choose a reason for hiding this comment

oliviertassinari Jul 15, 2024

Choose a reason for hiding this comment

oliviertassinari Jul 15, 2024

Choose a reason for hiding this comment

oliviertassinari Jul 15, 2024 • edited Loading

Choose a reason for hiding this comment

JCQuintas Jul 15, 2024

Choose a reason for hiding this comment

JCQuintas commented Jun 27, 2024 •

edited

Loading

mui-bot commented Jun 27, 2024 •

edited

Loading

JCQuintas commented Jul 3, 2024 •

edited

Loading

JCQuintas commented Jul 10, 2024 •

edited

Loading

oliviertassinari Jul 15, 2024 •

edited

Loading