[charts] Fix typescript error when using sx property on ChartsTooltip
#12659
Conversation
|
Deploy preview: https://deploy-preview-12659--material-ui-x.netlify.app/ |
|
Looks like an easy solution, but it does not go in the direction of not depending on The |
|
@alexfauquette good to know. I had initially added that, but saw the material Popper being used somewhere else and thought it was better to follow the pattern. I will fix it right away, thanks |
|
@alexfauquette Also, how should I label this PR? And is there any docs on how labelling works? |
JCQuintas
added
bug 🐛
| @@ -21,6 +21,13 @@ import { ChartsItemContentProps, ChartsItemTooltipContent } from './ChartsItemTo | |||
| import { ChartsAxisContentProps, ChartsAxisTooltipContent } from './ChartsAxisTooltipContent'; | |||
| import { ChartsTooltipClasses, getChartsTooltipUtilityClass } from './chartsTooltipClasses'; | |||
|
|
|||
| export type PopperProps = BasePopperProps & { | |||
There was a problem hiding this comment.
Nitpick: IMHO, this type could have also been prefixed with Charts to avoid the conflict with base and material types.
There was a problem hiding this comment.
That could be better indeed, is there a pattern we follow in such cases? Like, both material and base are PopperProps, so there is already some confusion there 🤔
There was a problem hiding this comment.
There are no hard rules and material/base are not an examples of making types unique.
For Charts we somewhat settled on trying to prefix types (especially generic ones) with Charts to avoid further confusion.
For example, we have the ChartsGridProps instead of GridProps to avoid potential errors. 🙈
Maybe @alexfauquette has a more clear vision/direction for type naming? 🤔
There was a problem hiding this comment.
Since the component name is ChartsTooltipRoot I assume we could have named it ChartsTooltipRootProps But those naming are not ideal. They would benefit an entire renaming to harmonize the notion of tooltip/popper
I do not expect user to use it, or if they do the confusion with the core type should not be a big deal since they are quite similar
The cause for this is that
@mui/base/Popperdoesn't seen to have the sx property.I've re-exported a
PopperPropsproperty with the addedsxprop. I decided on having the property be exported from the lib in order for users to be able to use it somewhere else.