[charts] Add missing themeAugmentation in pro plan

[lhilgert9]

• I have followed (at least) the PR section of the contributing guide.

Fixes #14304.

Preview: https://deploy-preview-14313--material-ui-x.netlify.app/x/react-charts/getting-started/#typescript

[mui-bot]

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

Updated pages:

• docs/data/charts/getting-started/getting-started.md

Generated by 🚫 dangerJS against bc59def

[JCQuintas]

Thanks for the improvement. 💪

This looks good to me, I'll just ask my colleague @alexfauquette for a final check. :)

[alexfauquette]

Nice improvement. There are just two points we need to verify before merging:

• Is the theme augmentation effectively working?
• Since this PR impacts a non MIT library, we light need you to sign an agreement. I'm not sure about the exact process.' @michelengelen do you have more information

[michelengelen]

Nice improvement. There are just two points we need to verify before merging:

• Is the theme augmentation effectively working?

• Since this PR impacts a non MIT library, we light need you to sign an agreement. I'm not sure about the exact process.' @michelengelen do you have more information

@joserodolfofreitas would you mind helping out with the CLA?

[lhilgert9]

Nice improvement. There are just two points we need to verify before merging:

• Is the theme augmentation effectively working?
• Since this PR impacts a non MIT library, we light need you to sign an agreement. I'm not sure about the exact process.' @michelengelen do you have more information

@joserodolfofreitas would you mind helping out with the CLA?

@michelengelen I must have signed the CLA last year or so, if it is still valid.

[codspeed-hq]

CodSpeed Performance Report

Merging #14313 will not alter performance

_{Comparing lhilgert9:charts-pro/add-theme-augmentation (bc59def) with master (1080a76)}

Summary

✅ 3 untouched benchmarks

[lhilgert9]

• Is the theme augmentation effectively working?
  @alexfauquette Should I create an own test file for themeAugmentation or test it in the heatmap test files or how did you imagine that?

[oliviertassinari]

Since this PR impacts a non MIT library, we light need you to sign an agreement. I'm not sure about the exact process.'

The process for maintainers is documented alongside the process for contributors: https://www.notion.so/mui-org/CLA-Contributor-License-Agreement-92ece655b1584b10b00e4de9e67eedb0?pvs=4#417626432f9e419f9fc8bc90aa6ca871.

I must have signed the CLA last year or so, if it is still valid.

@lhilgert9 We have an entry for it, but couldn't verify your email. Could you do this step? https://www.notion.so/mui-org/CLA-Contributor-License-Agreement-92ece655b1584b10b00e4de9e67eedb0?pvs=4#fb0396ecff384a4cb515ab9f4351eddc Thanks

[lhilgert9]

@oliviertassinari Your mail server has rejected the forwarded message as spam. What can I do instead?

[oliviertassinari]

@lhilgert9 Ok, thanks, that should be enough in this case, we can clearly correlate the signature form timestamp and link with the GitHub profile.

[lhilgert9]

@alexfauquette @JCQuintas @oliviertassinari While testing a bit with the themeAugmentation, I came to the point that the themeAugmentation for the normal charts package doesn't work either. It seems to be because the useThemeProps hook is not used as with all other components at mui. Is this intentional?

[alexfauquette]

Is that intentional

Not really. It's because we are in the first version of the library, and did not find time to figure out which component should have this option. We have #12070 to track it

[lhilgert9]

@alexfauquette My code seems to work on its own. However, all components of the pro plan are not yet equipped with the necessary functions for themeAugmentation, which is why the actual function is currently ineffective. Do we want to implement these functions in this PR or should I open another PR for the heatmap, for example, and do we want to put this PR on hold for the time being?

[alexfauquette]

@lhilgert9 I'm working on fixing it.

I propose to add the default props to all the single components (LineChart, BarChart, ... LineChartPro, ..., Heatmap)
Not to the subcomponents (LinePlot, LineElement, ..., HeatmapPlot, HeatmapItem) because those components props are just SVG attributes, then modifying them is either for style customization (this can be done with CSS) or custom logic that I would not recommend doing with defaultProps

While working on it, I'm cleaning the TS to remove type definition of stuff that don't work

[lhilgert9]

@alexfauquette If I can help anywhere, please feel free to let me know.

[github-actions]

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

[alexfauquette]

I did a big chunk of cleaning the TS definition.

I propose we merge this PR to get the big picture merged, and in follow-up PR add conformance test, to make sure each coponent have the default Props and styling applied as expected.

[lhilgert9]

@alexfauquette The plan sounds good to me. However, I have changed the implementation of the useThemeProps hook in the code as follows to be compliant with the other packages:

function Component(inProps: ComponentProps) {
const props = useThemeProps({props: inProps, name: 'MuiComponentName'});
// implementation
}

The rest of the changes LGTM.

[oliviertassinari]

I'm unclear about this change. Does [`.${axisClasses.line}`]: { target the same element or a child element? I always find this API confusing, I can never remember. Always having & would mean that the developers looking at the source to see what to customize with CSS modules never have doubts. It feels like we should have this rule.

[alexfauquette]

My issue is that we do not styled() the sub part of the axis, to avoid styling to many elements.

The idea of this modification was to reduce the specificity of the default style to let people override it from the root with & .${axisClasses.line}

[oliviertassinari]

Does it mean its equivalent to?

  [`&.${axisClasses.line}`]: {

[alexfauquette]

I got confused. I thaugh that removing the & would remove the root class. But it does not.

This modification changes nothing. I just tried the before after:

[`& .${axisClasses.line}`]: { ... },
[`.${axisClasses.line}`]: { },

The both selector above result into the same CSS

.css-6ic30x-MuiChartsAxis-root .MuiChartsAxis-line { ... } // in dev env
.css-6ic30x .MuiChartsAxis-line { ... } // in prod env

[oliviertassinari]

Oh ok, this is what I was wondering about:

is [`.${axisClasses.line}`]: { }, equivalent to [`& .${axisClasses.line}`]: { ... }, or [`&.${axisClasses.line}`]: { ... },. It's the former, thanks I was too lazy to test 😄

I think we should always have & in the source then, no doubts possible.

[alexfauquette]

Fix in #14378