-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
[charts] Fix themeAugmentation #14372
Conversation
const XAxisRoot = styled(AxisRoot, { | ||
name: 'MuiChartsXAxis', | ||
slot: 'Root', | ||
overridesResolver: (props, styles) => styles.root, | ||
})({}); |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
Deploy preview: https://deploy-preview-14372--material-ui-x.netlify.app/ |
/** | ||
* 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']; |
There was a problem hiding this comment.
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} />} |
There was a problem hiding this comment.
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:
/** | |
* 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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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} />
There was a problem hiding this comment.
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}> |
There was a problem hiding this comment.
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
CodSpeed Performance ReportMerging #14372 will not alter performanceComparing Summary
|
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
Signed-off-by: Jose C Quintas Jr <[email protected]>
Follow up of #14313
I was looking to add
describeConformance
but those test make assumptions that are not respected by charts. For example it expectsChartsXAxis
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