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

Changes from 1 commit
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
5 changes: 5 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 @@ -16,6 +16,11 @@ export interface ChartsComponents<Theme = unknown> {
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