feat(plugin-chart-echarts): subject Update echarts to v5.0.1#928
feat(plugin-chart-echarts): subject Update echarts to v5.0.1#928villebro merged 11 commits intoapache-superset:masterfrom
Conversation
|
This pull request is being automatically deployed with Vercel (learn more). 🔍 Inspect: https://vercel.com/superset/superset-ui/ez273hb7j |
Codecov Report
@@ Coverage Diff @@
## master #928 +/- ##
==========================================
+ Coverage 27.43% 27.46% +0.03%
==========================================
Files 412 412
Lines 8300 8304 +4
Branches 1147 1147
==========================================
+ Hits 2277 2281 +4
Misses 5889 5889
Partials 134 134
Continue to review full report at Codecov.
|
villebro
left a comment
villebro
left a comment
There was a problem hiding this comment.
Great work! 🎉 A few comments/questions - they might not all be applicable as I haven't looked at the new types, but would be curious to hear your thoughts on these.
| getMetricLabel, | ||
| getNumberFormatter, | ||
| } from '@superset-ui/core'; | ||
| import { EChartsOption, SeriesOption } from 'echarts'; |
There was a problem hiding this comment.
Is there something like BoxplotSeriesOption?
| // @ts-ignore | ||
| ...outlierData, | ||
| ], | ||
| ] as SeriesOption, |
There was a problem hiding this comment.
I would prefer not to cast this but rather @ts-ignore any specific typing problems that absolutely can't be avoided. I think this needs to be done separately like so:
const series: BoxplotSeriesOption = [...];and then later
const echartsOptions: EChartsOption = {
...,
series,
...,
}There was a problem hiding this comment.
done for box and pie chart
| data: transformedData, | ||
| tooltip: { | ||
| formatter: param => { | ||
| formatter: (param: CallbackDataParams) => { |
There was a problem hiding this comment.
This looks ok, just curious if there could be a dedicated type for the tooltip formatter params?
There was a problem hiding this comment.
Not really, formatter does not have a dedicated type
villebro
left a comment
villebro
left a comment
There was a problem hiding this comment.
Looking pretty good! Can you check those few comments, after that I think we can merge this and do additional fine tuning in forthcoming PRs.
|
|
||
| const transformedData = data | ||
| .map(datum => { | ||
| const transformedData: (BoxplotDataItemOption & { name: string })[] = data |
There was a problem hiding this comment.
I'm curious name isn't in the BoxplotDataItemOption interface/type. Can you check if it should be called something else? If not, let's keep the type unchanged and just @ts-ignore the line.
| .flatMap(row => row); | ||
|
|
||
| const outlierData = data | ||
| const outlierData: (BoxplotStateOption & { name: string })[] = data |
3 participants
Update echarts package to version 5.0.1 and change types.