Skip to content
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.

Sign up for GitHub

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

[charts] Fix themeAugmentation #14372

Merged
merged 8 commits into from
Aug 29, 2024
Merged

[charts] Fix themeAugmentation #14372

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
e0293d7
ALlow style override on x/y axis
alexfauquette Aug 28, 2024
b4a3929
Fix legend
alexfauquette Aug 28, 2024
ce61cbd
update grid
alexfauquette Aug 28, 2024
fce8a7e
update tooltip
alexfauquette Aug 28, 2024
f73dcd8
fix tooltip default
alexfauquette Aug 28, 2024
f2e01fc
scripts
alexfauquette Aug 28, 2024
4b6e8eb
Rework ChartsGrid
alexfauquette Aug 29, 2024
690f1be
Merge branch 'master' into continue-yhemeOverride
JCQuintas Aug 29, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 10 additions & 9 deletions packages/x-charts/src/ChartsLegend/ChartsLegend.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,20 +59,21 @@ const useUtilityClasses = (ownerState: DefaultizedChartsLegendProps & { theme: T
return composeClasses(slots, getLegendUtilityClass, classes);
};

const defaultProps = {
position: { horizontal: 'middle', vertical: 'top' },
direction: 'row',
} as const;

function ChartsLegend(inProps: ChartsLegendProps) {
const props: DefaultizedChartsLegendProps = useThemeProps({
props: { ...defaultProps, ...inProps },
const props = useThemeProps({
props: inProps,
name: 'MuiChartsLegend',
});
JCQuintas marked this conversation as resolved.
Show resolved Hide resolved

const { position, direction, hidden, slots, slotProps } = props;
const defaultizedProps: DefaultizedChartsLegendProps = {
direction: 'row',
...props,
position: { horizontal: 'middle', vertical: 'top', ...props.position },
};
const { position, direction, hidden, slots, slotProps } = defaultizedProps;

const theme = useTheme();
const classes = useUtilityClasses({ ...props, theme });
const classes = useUtilityClasses({ ...defaultizedProps, theme });

const drawingArea = useDrawingArea();
const series = useSeries();
Expand Down
13 changes: 7 additions & 6 deletions packages/x-charts/src/ChartsTooltip/ChartsTooltip.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,6 +94,7 @@ const useUtilityClasses = <T extends ChartSeriesType>(ownerState: {

const slots = {
root: ['root'],
paper: ['paper'],
table: ['table'],
row: ['row'],
cell: ['cell'],
Expand Down Expand Up @@ -124,12 +125,12 @@ const ChartsTooltipRoot = styled(Popper, {
*
* - [ChartsTooltip API](https://mui.com/x/api/charts/charts-tool-tip/)
*/
function ChartsTooltip<T extends ChartSeriesType>(props: ChartsTooltipProps<T>) {
const themeProps = useThemeProps({
props,
function ChartsTooltip<T extends ChartSeriesType>(inProps: ChartsTooltipProps<T>) {
const props = useThemeProps({
props: inProps,
name: 'MuiChartsTooltip',
});
const { trigger = 'axis', itemContent, axisContent, slots, slotProps } = themeProps;
const { trigger = 'axis', itemContent, axisContent, slots, slotProps } = props;

const mousePosition = useMouseTracker();

Expand All @@ -140,7 +141,7 @@ function ChartsTooltip<T extends ChartSeriesType>(props: ChartsTooltipProps<T>)
const tooltipHasData = getTooltipHasData(trigger, displayedData);
const popperOpen = mousePosition !== null && tooltipHasData;

const classes = useUtilityClasses({ classes: themeProps.classes });
const classes = useUtilityClasses({ classes: props.classes });

const PopperComponent = slots?.popper ?? ChartsTooltipRoot;
const popperProps = useSlotProps({
Expand Down Expand Up @@ -170,7 +171,7 @@ function ChartsTooltip<T extends ChartSeriesType>(props: ChartsTooltipProps<T>)
return (
<NoSsr>
{popperOpen && (
<PopperComponent {...popperProps}>
<PopperComponent {...popperProps} className={classes.root}>
Copy link
Member Author

Choose a reason for hiding this comment

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

Before that change, the PopperComponent has the styleOverride.root, but the classes.root was on the ChartsTooltipPaper

With that modification both styleOverrides.root and classes.root are on the same component

{trigger === 'item' ? (
<ChartsItemTooltipContent
itemData={displayedData as ItemInteractionData<T>}
Expand Down
5 changes: 5 additions & 0 deletions packages/x-charts/src/ChartsTooltip/ChartsTooltipTable.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import { chartsTooltipClasses } from './chartsTooltipClasses';
export const ChartsTooltipPaper = styled('div', {
name: 'MuiChartsTooltip',
slot: 'Container',
overridesResolver: (props, styles) => styles.paper,
})(({ theme }) => ({
boxShadow: theme.shadows[1],
backgroundColor: (theme.vars || theme).palette.background.paper,
Expand All @@ -22,6 +23,7 @@ export const ChartsTooltipPaper = styled('div', {
export const ChartsTooltipTable = styled('table', {
name: 'MuiChartsTooltip',
slot: 'Table',
overridesResolver: (props, styles) => styles.table,
})(({ theme }) => ({
borderSpacing: 0,
'& thead td': {
Expand All @@ -35,6 +37,7 @@ export const ChartsTooltipTable = styled('table', {
export const ChartsTooltipRow = styled('tr', {
name: 'MuiChartsTooltip',
slot: 'Row',
overridesResolver: (props, styles) => styles.row,
})(({ theme }) => ({
'tr:first-of-type& td': {
paddingTop: theme.spacing(1),
Expand All @@ -50,6 +53,7 @@ export const ChartsTooltipRow = styled('tr', {
export const ChartsTooltipCell = styled('td', {
name: 'MuiChartsTooltip',
slot: 'Cell',
overridesResolver: (props, styles) => styles.cell,
})(({ theme }) => ({
verticalAlign: 'middle',
color: (theme.vars || theme).palette.text.secondary,
Expand All @@ -74,6 +78,7 @@ export const ChartsTooltipCell = styled('td', {
export const ChartsTooltipMark = styled('div', {
name: 'MuiChartsTooltip',
slot: 'Mark',
overridesResolver: (props, styles) => styles.mark,
shouldForwardProp: (prop) => shouldForwardProp(prop) && prop !== 'color',
})<{ color: string }>(({ theme, color }) => ({
width: theme.spacing(1),
Expand Down
2 changes: 1 addition & 1 deletion packages/x-charts/src/ChartsTooltip/DefaultChartsAxisTooltipContent.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ function DefaultChartsAxisTooltipContent(props: ChartsAxisContentProps) {
axis.scaleType === 'utc' ? utcFormatter(v) : v.toLocaleString());

return (
<ChartsTooltipPaper sx={sx} className={classes.root}>
<ChartsTooltipPaper sx={sx} className={classes.paper}>
<ChartsTooltipTable className={classes.table}>
{axisValue != null && !axis.hideTooltip && (
<thead>
Expand Down
2 changes: 1 addition & 1 deletion packages/x-charts/src/ChartsTooltip/DefaultChartsItemTooltipContent.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ function DefaultChartsItemTooltipContent<T extends ChartSeriesType = ChartSeries
series.valueFormatter as CommonSeriesType<typeof value>['valueFormatter']
)?.(value, { dataIndex: itemData.dataIndex });
return (
<ChartsTooltipPaper sx={sx} className={classes.root}>
<ChartsTooltipPaper sx={sx} className={classes.paper}>
<ChartsTooltipTable className={classes.table}>
<tbody>
<ChartsTooltipRow className={classes.row}>
Expand Down
4 changes: 3 additions & 1 deletion packages/x-charts/src/ChartsTooltip/chartsTooltipClasses.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,8 @@ import {
export interface ChartsTooltipClasses {
/** Styles applied to the root element. */
root: string;
/** Styles applied to the paper element. */
paper: string;
/** Styles applied to the table element. */
table: string;
/** Styles applied to the row element. */
Expand Down Expand Up @@ -33,5 +35,5 @@ export function getChartsTooltipUtilityClass(slot: string) {
}
export const chartsTooltipClasses: ChartsTooltipClasses = generateUtilityClasses(
'MuiChartsTooltip',
['root', 'table', 'row', 'cell', 'mark', 'markCell', 'labelCell', 'valueCell'],
['root', 'paper', 'table', 'row', 'cell', 'mark', 'markCell', 'labelCell', 'valueCell'],
);
12 changes: 9 additions & 3 deletions packages/x-charts/src/ChartsXAxis/ChartsXAxis.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ import * as React from 'react';
import PropTypes from 'prop-types';
import useSlotProps from '@mui/utils/useSlotProps';
import composeClasses from '@mui/utils/composeClasses';
import { useThemeProps, useTheme, Theme } from '@mui/material/styles';
import { useThemeProps, useTheme, Theme, styled } from '@mui/material/styles';
import { useCartesianContext } from '../context/CartesianProvider';
import { useTicks, TickItemType } from '../hooks/useTicks';
import { AxisDefaultized, ChartsXAxisProps } from '../models/axis';
Expand Down Expand Up @@ -83,6 +83,12 @@ function addLabelDimension(
});
}

const XAxisRoot = styled(AxisRoot, {
name: 'MuiChartsXAxis',
slot: 'Root',
overridesResolver: (props, styles) => styles.root,
})({});
Comment on lines +86 to +90
Copy link
Member Author

Choose a reason for hiding this comment

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

Looks like a bit of overwork to style a component already styled. But it improve consistency.

I'm considering removing the AxisRoot for v8. WHat do you think?

Copy link
Member

Choose a reason for hiding this comment

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

What do you mean by removing it? Move the styles to each of the axis?


const defaultProps = {
position: 'bottom',
disableLine: false,
Expand Down Expand Up @@ -204,7 +210,7 @@ function ChartsXAxis(inProps: ChartsXAxisProps) {
return null;
}
return (
<AxisRoot
<XAxisRoot
transform={`translate(0, ${position === 'bottom' ? top + height : top})`}
className={classes.root}
sx={sx}
Expand Down Expand Up @@ -246,7 +252,7 @@ function ChartsXAxis(inProps: ChartsXAxisProps) {
<Label {...labelRefPoint} {...axisLabelProps} text={label} />
</g>
)}
</AxisRoot>
</XAxisRoot>
);
}

Expand Down
12 changes: 9 additions & 3 deletions packages/x-charts/src/ChartsYAxis/ChartsYAxis.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ import * as React from 'react';
import PropTypes from 'prop-types';
import useSlotProps from '@mui/utils/useSlotProps';
import composeClasses from '@mui/utils/composeClasses';
import { useThemeProps, useTheme, Theme } from '@mui/material/styles';
import { useThemeProps, useTheme, Theme, styled } from '@mui/material/styles';
import { useCartesianContext } from '../context/CartesianProvider';
import { useTicks } from '../hooks/useTicks';
import { useDrawingArea } from '../hooks/useDrawingArea';
Expand All @@ -27,6 +27,12 @@ const useUtilityClasses = (ownerState: ChartsYAxisProps & { theme: Theme }) => {
return composeClasses(slots, getAxisUtilityClass, classes);
};

const YAxisRoot = styled(AxisRoot, {
name: 'MuiChartsYAxis',
slot: 'Root',
overridesResolver: (props, styles) => styles.root,
})({});

const defaultProps = {
position: 'left',
disableLine: false,
Expand Down Expand Up @@ -156,7 +162,7 @@ function ChartsYAxis(inProps: ChartsYAxisProps) {
}

return (
<AxisRoot
<YAxisRoot
transform={`translate(${position === 'right' ? left + width : left}, 0)`}
className={classes.root}
sx={sx}
Expand Down Expand Up @@ -203,7 +209,7 @@ function ChartsYAxis(inProps: ChartsYAxisProps) {
<Label {...labelRefPoint} {...axisLabelProps} text={label} />
</g>
)}
</AxisRoot>
</YAxisRoot>
);
}

Expand Down
7 changes: 7 additions & 0 deletions packages/x-charts/src/themeAugmentation/components.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,14 +6,21 @@ export interface ChartsComponents<Theme = unknown> {
};
MuiChartsXAxis?: {
defaultProps?: ComponentsProps['MuiChartsXAxis'];
styleOverrides?: ComponentsOverrides<Theme>['MuiChartsXAxis'];
};
MuiChartsYAxis?: {
defaultProps?: ComponentsProps['MuiChartsYAxis'];
styleOverrides?: ComponentsOverrides<Theme>['MuiChartsYAxis'];
};
MuiChartsAxisHighlight?: {
styleOverrides?: ComponentsOverrides<Theme>['MuiChartsAxisHighlight'];
};
MuiChartsGrid?: {
/**
* Warning, does not work with LineChart, BarChart and ScatterChart.
* Those components skip the grid rendering if it seems unnecessary according to there props.
* This `defaultProps` only work if you call `<ChartsGrid />` in a composed chart.
*/
defaultProps?: ComponentsProps['MuiChartsGrid'];
Copy link
Member Author

Choose a reason for hiding this comment

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

Not sure it's supper clear. I'm trying to explain that the defaultProps of this components would have no impact on single component charts if grid is not defined, because of this line

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

Line 133 in 97afd65

{props.grid && <ChartsGrid {...gridProps} />}

Copy link
Member

Choose a reason for hiding this comment

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

Maybe keep it very simple, like:

Suggested change
/**
* Warning, does not work with LineChart, BarChart and ScatterChart.
* Those components skip the grid rendering if it seems unnecessary according to there props.
* This `defaultProps` only work if you call `<ChartsGrid />` in a composed chart.
*/
defaultProps?: ComponentsProps['MuiChartsGrid'];
/**
* The `MuiChartsGrid.defaultProps` only work when using [composition](/x/react-charts/composition/).
*/
defaultProps?: ComponentsProps['MuiChartsGrid'];

An alternative is to have the charts always call <ChartsGrid /> and chart grid resolves the theme props and decides if it needs to render or not.

Copy link
Member Author

Choose a reason for hiding this comment

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

An alternative is to have the charts always call and chart grid resolves the theme props and decides if it needs to render or not.

Rendering the grid is not free. Removing it saves 10ms on rendering. Would need to verify if it's the hooks computation, or the rendering which is responsible

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, but we could try to create a proxy renderer eg:

function Proxy(inProps) {
  const props = useThemeProps({ props: inProps, name: 'MuiChartsGrid' });
  if (!props.checkExistanceSomehow) return null
  return <ChartsGrid {...props} />

Copy link
Member Author

Choose a reason for hiding this comment

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

I went the other way by creating sub-components for vertical and horizontal. Such that useTick is called only if the result will be render.

styleOverrides?: ComponentsOverrides<Theme>['MuiChartsGrid'];
};
Expand Down
10 changes: 6 additions & 4 deletions packages/x-charts/src/themeAugmentation/overrides.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,19 +2,21 @@ import { BarLabelClassKey } from '../BarChart';
import { BarElementClassKey } from '../BarChart/BarElement';
import { ChartsAxisHighlightClassKey } from '../ChartsAxisHighlight';
import { ChartsGridClassKey } from '../ChartsGrid';
import { ChartsLegendClassKey } from '../ChartsLegend';

import { ChartsTooltipClassKey } from '../ChartsTooltip';
import { AreaElementClassKey, LineElementClassKey, MarkElementClassKey } from '../LineChart';

export interface ChartsComponentNameToClassKey {
MuiChartsAxis: 'root'; // Only the root component of axes is styled
MuiChartsAxis: 'root'; // Only the root component of axes is styled. We should probably remove this one in v8
MuiChartsXAxis: 'root'; // Only the root component of axes is styled
MuiChartsYAxis: 'root'; // Only the root component of axes is styled

MuiChartsAxisHighlight: ChartsAxisHighlightClassKey;
MuiChartsLegend: 'root';
MuiChartsGrid: ChartsGridClassKey;
MuiChartsLegend: ChartsLegendClassKey;
MuiChartsTooltip: ChartsTooltipClassKey;

MuiChartsSurface: 'root';

// BarChart components
MuiBarElement: BarElementClassKey;
MuiBarLabel: BarLabelClassKey;
Expand Down
12 changes: 11 additions & 1 deletion packages/x-charts/src/themeAugmentation/themeAugmentation.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,13 +15,23 @@ createTheme({
// @ts-expect-error invalid MuiChartsXAxis prop
someRandomProp: true,
},
styleOverrides: {
root: { backgroundColor: 'red' },
// @ts-expect-error invalid MuiChartsXAxis class key
line: { color: 'red' },
},
},
MuiChartsYAxis: {
defaultProps: {
axisId: 'test',
// @ts-expect-error invalid MuiChartsYAxis prop
someRandomProp: true,
},
styleOverrides: {
root: { backgroundColor: 'red' },
// @ts-expect-error invalid MuiChartsYAxis class key
line: { color: 'red' },
},
},
MuiChartsAxisHighlight: {
styleOverrides: {
Expand All @@ -39,7 +49,7 @@ createTheme({
styleOverrides: {
root: { backgroundColor: 'red' },
// @ts-expect-error invalid MuiChartsLegend class key
constent: { color: 'red' },
mark: { color: 'red' },
},
},
MuiChartsTooltip: {
Expand Down