-
Notifications
You must be signed in to change notification settings - Fork 272
feat(plugin-chart-echarts): Radar chart POC #1029
Conversation
This pull request is being automatically deployed with Vercel (learn more). 🔍 Inspect: https://vercel.com/superset/superset-ui/7seHzEGan2vQpZ6GpyUWZq5vvk95 |
@kgabryje Kamil, please help review this PR 🙏 we are hoping to get it in by the end of next week with all comments are addressed. |
renderTrigger: true, | ||
default: numberFormat, | ||
choices: D3_FORMAT_OPTIONS, | ||
description: `${t('D3 format syntax: https://github.com/d3/d3-format')} ${t( |
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.
I think we should add a new line between the link to D3 and the next sentence
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.
In the control description looks that can not break lines.
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.
How about adding a dot at the end of first sentence?
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.
description: `${t('D3 format syntax: https://github.com/d3/d3-format')} ${t( | |
description: `${t('D3 format syntax: https://github.com/d3/d3-format.')} ${t( |
Can you please bump superset-ui-core and chart-controls versions, so that #1035 is included? |
2cb879c
to
3d24cd3
Compare
6717fa1
to
780f720
Compare
Codecov Report
@@ Coverage Diff @@
## master #1029 +/- ##
==========================================
- Coverage 28.13% 27.94% -0.20%
==========================================
Files 441 447 +6
Lines 8939 9019 +80
Branches 1363 1384 +21
==========================================
+ Hits 2515 2520 +5
- Misses 6236 6310 +74
- Partials 188 189 +1
Continue to review full report at Codecov.
|
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! Pointed 1 tiny nit, but it's good without it 🙂
I noticed a bug - see comment below
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.
There's a bug in "Customize metrics" section. If you click on a value related to "group by", an exception is thrown (see video)
Screen.Recording.2021-04-16.at.10.11.43.mov
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!
🏆 Enhancements
Added Radar Chart
Additional
This PR dependence: apache/superset#13983
Data Panel
Cross filter
radar.cross.filter.mp4
Config Metric
each.metric.mp4