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.

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

Conversation

alexfauquette
Copy link
Member

Follow up of #14313

I was looking to add describeConformance but those test make assumptions that are not respected by charts. For example it expects ChartsXAxis to propagate its props to the root element. Whereas the component take the needed value from its props and render the axis.

In this PR I took care of the general components. Each component has its dedicated commit

@dosubot dosubot bot added the size:L This PR changes 100-499 lines, ignoring generated files. label Aug 28, 2024
Comment on lines +86 to +90
const XAxisRoot = styled(AxisRoot, {
name: 'MuiChartsXAxis',
slot: 'Root',
overridesResolver: (props, styles) => styles.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.

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?

@mui-bot
Copy link

mui-bot commented Aug 28, 2024

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

Generated by 🚫 dangerJS against 690f1be

Comment on lines 19 to 24
/**
* 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

{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.

@@ -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

@alexfauquette alexfauquette added the component: charts This is the name of the generic UI component, not the React module! label Aug 28, 2024
Copy link

codspeed-hq bot commented Aug 28, 2024

CodSpeed Performance Report

Merging #14372 will not alter performance

Comparing alexfauquette:continue-yhemeOverride (690f1be) with master (70b6cb7)

Summary

✅ 3 untouched benchmarks

Copy link

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

@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Aug 29, 2024
alexfauquette and others added 2 commits August 29, 2024 13:58
Signed-off-by: Jose C Quintas Jr <juniorquintas@gmail.com>
@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Aug 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: charts This is the name of the generic UI component, not the React module! size:L This PR changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants