[charts] Remove colors prop from SparkLineChart.
#16494
Conversation
| <SparkLineChart data={data} colors={['red']} /> | ||
| <SparkLineChart data={data} colors={fn} /> | ||
| <SparkLineChart data={data} colors={(mode) => (mode === 'light' ? ['black'] : ['white'])} /> | ||
| <SparkLineChart data={data} colors="red" /> |
There was a problem hiding this comment.
This is valid since colors can be an array of strings or a function, but added it to ensure that we handle invalid well (i.e., by not applying any transformation).
There was a problem hiding this comment.
Removed this test case because TypeScript wasn't happy with invalid properties.
bernardobelchior
added
breaking change
component: charts
|
Deploy preview: https://deploy-preview-16494--material-ui-x.netlify.app/ |
CodSpeed Performance ReportMerging #16494 will not alter performanceComparing Summary
|
| (<React.Fragment> | ||
| <SparkLineChart data={data} color={['red'][0]} /> | ||
| <SparkLineChart data={data} color={typeof fn === "function" ? mode => fn(mode)?.[0] : fn} /> | ||
| <SparkLineChart data={data} color={mode => (mode => (mode === 'light' ? ['black'] : ['white']))(mode)?.[0]} /> |
There was a problem hiding this comment.
This might seem a bit confusing, but what I'm doing is:
Given an arrow function declaration fnDecl, transform it into: mode => (fnDecl)(mode)?.[0].
We need the optional chaining operator because undefined is a valid value for colors, so we need be careful not to access [0] of an undefined value.
| return ( | ||
| (<React.Fragment> | ||
| <SparkLineChart data={data} color={['red'][0]} /> | ||
| <SparkLineChart data={data} color={typeof fn === "function" ? mode => fn(mode)?.[0] : fn} /> |
There was a problem hiding this comment.
Not sure if this is a valid approach.
This introduces a slight runtime performance hit because, when codemodding, we can't guarantee whether fn is a function or an array.
If you think this or another example are too complex, I can just remove this and leave this as an unhandled case.
| // Don't know how to handle this case | ||
| } | ||
|
|
||
| // Only apply transformation if we know how to handle it |
There was a problem hiding this comment.
Is there any best practice for letting the user know where we caught an unhandled case? I know we aren't covering 100% of the cases, and some are easy to detect, but hard to fix. It would be nice if we could let the user know which ones have been detected to guide them towards where they need to do a manual fix.
There was a problem hiding this comment.
I don't think we currently notify unhandled cases
| // prettier-ignore | ||
| return ( | ||
| (<React.Fragment> | ||
| <SparkLineChart data={data} color={['red']?.[0]} /> |
There was a problem hiding this comment.
I followed a "better safe than sorry" approach here. Adding the optional chain operator might save us from some crashes in edge cases. Happy to remove it if you think that's too much.
There was a problem hiding this comment.
what will happen to var when var=['blue']?
bernardobelchior
force-pushed
the
remove-sparkline-colors
branch
from
936f04b to
e25f08c
Compare
github-actions
bot
added
the
PR: out-of-date
|
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
…o appease TypeScript.
bernardobelchior
force-pushed
the
remove-sparkline-colors
branch
from
e25f08c to
237df3b
Compare
github-actions
bot
removed
the
PR: out-of-date
Remove
colorsprop fromSparkLineChart.In #16477, I deprecated the
colorsprop, but we can still do breaking changes for v8. This will reduce the maintenance burden slightly.Also added codemod to migrate from the old
colorsprop to the newcolorprop.