-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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] Improve dataset typing #11372
Conversation
Deploy preview: https://deploy-preview-11372--material-ui-x.netlify.app/ |
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.
It's a nice improvement! 🙌
<...> I'm wondering if the default type should be unknown such that they can put whatever they want in the dataset, and do stricter type-checking into formatters
That's a very valid proposition. 👍
However, do you feel it is a problem now?
Because if it's not, then maybe we can wait for community confirmation (request for it) on this regard?
It should be too hard making the types more loose—possibly not a breaking change. 😃
import defaultizeValueFormatter from '../internals/defaultizeValueFormatter'; | ||
import { DefaultizedProps } from '../models/helpers'; | ||
|
||
let warnOnce = false; |
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.
let warnOnce = false; | |
let warnedOnce = false; |
if (process.env.NODE_ENV !== 'production' && !warnOnce && value !== null) { | ||
warnOnce = true; | ||
console.error([ | ||
`MUI-X charts: your dataset key "${dataKey}" is used for plotting line, but contains non number elements.`, |
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.
`MUI-X charts: your dataset key "${dataKey}" is used for plotting line, but contains non number elements.`, | |
`MUI-X charts: your dataset key "${dataKey}" is used for plotting line, but contains nonnumerical elements.`, |
if (process.env.NODE_ENV !== 'production' && !warnOnce && value !== null) { | ||
warnOnce = true; | ||
console.error([ | ||
`MUI-X charts: your dataset key "${dataKey}" is used for plotting bars, but contains non number elements.`, |
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.
`MUI-X charts: your dataset key "${dataKey}" is used for plotting bars, but contains non number elements.`, | |
`MUI-X charts: your dataset key "${dataKey}" is used for plotting bars, but contains nonnumerical elements.`, |
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.
lgtm 😅😇
When working on mui/material-ui#40107 I noticed
null
was not accepted as a data set value. This PR fixes this issue by allowingnull
andundefined
.On a larger topic, I think devs tend to put various things in the dataset. The result of their data fetching without preprocessing to enable rich tooltip for example. I'm wondering if the default type should be
unknown
such that they can put whatever they want in the dataset, and do stricter type-checking into formatters