fix(plugin-chart-echarts): improve ECharts visuals by villebro · Pull Request #806 · apache-superset/superset-ui

villebro · 2020-10-13T08:49:07Z

🏆 Enhancements

Improve ECharts plugin visuals. Changes:

Pie

Disable outside label line by default and add a control to enable it. Feature parity with NVD3 Pie.
Change label color to black by default (looks less messy when labels inside). Feature parity with NVD3 Pie.
Added customizable minimum percentage threshold (default: 5 %) to be able to remove labels for thin slices. Feature parity with NVD3 Pie.
hover now doesn't change font size, only bolds the text (less invasive). Feature parity with NVD3 Pie.

Timeseries

Add y-axis truncate option (default: enabled). Feature parity with NVD3 Line.
Add y-axis bounds (min, max). Feature parity with NVD3 Line.

Storybook

Add Pie Chart case (population) with multiple thin slices to demonstrate minimum percentage threshold.
Add missing controls to Pie and Timeseries.

Screenshot from Storybook demonstrating lower threshold with lots of thin slices:

Same with labels outside:

Screenshot of chart where a slice below the threshold is hovered above.

Truncate and bounds on y-axis:

vercel · 2020-10-13T08:49:13Z

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/superset/superset-ui/ilmqpv9ot
✅ Preview: https://superset-ui-git-villebro-yaxis-scale.superset.vercel.app

codecov · 2020-10-13T10:50:47Z

Codecov Report

Merging #806 into master will increase coverage by 0.07%.
The diff coverage is 61.11%.

@@            Coverage Diff             @@
##           master     #806      +/-   ##
==========================================
+ Coverage   25.50%   25.57%   +0.07%     
==========================================
  Files         358      359       +1     
  Lines        7937     7949      +12     
  Branches     1051     1056       +5     
==========================================
+ Hits         2024     2033       +9     
- Misses       5792     5796       +4     
+ Partials      121      120       -1

Impacted Files	Coverage Δ
...plugin-chart-echarts/src/BoxPlot/transformProps.ts	`52.63% <ø> (ø)`
...ugin-chart-echarts/src/Timeseries/controlPanel.tsx	`100.00% <ø> (ø)`
...gin-chart-echarts/src/Timeseries/transformProps.ts	`27.08% <20.00%> (+0.41%)`	⬆️
...ins/plugin-chart-echarts/src/Pie/transformProps.ts	`73.80% <57.14%> (+0.12%)`	⬆️
plugins/plugin-chart-echarts/src/Pie/buildQuery.ts	`100.00% <100.00%> (ø)`
...ugins/plugin-chart-echarts/src/Pie/controlPanel.ts	`100.00% <100.00%> (ø)`
plugins/plugin-chart-echarts/src/defaults.ts	`100.00% <100.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7671c65...674d1d6. Read the comment docs.

mistercrunch · 2020-10-14T04:19:34Z

plugins/plugin-chart-echarts/src/Pie/controlPanel.ts

+          {
+            name: 'show_labels_threshold',
+            config: {
+              type: 'TextControl',


NIT(optional/out-of-scope-for-this-pr): slider could be cool here eventually. I just discovered how cool the antd sliders are! https://ant.design/components/slider/

Oh yeah, that Slider with input number looks perfect. I tinkered with the existing slider control, but it felt clunky as it doesn't allow for setting decimals, which in this case is often relevant. Let's refactor the SliderControl to use AntD in a follow-up PR and change this when that's in place.

mistercrunch · 2020-10-14T04:34:22Z

plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts

+  let [yAxisMin, yAxisMax] = yAxisBounds || [];
+  if (yAxisMin === undefined && contributionMode === 'row' && stack) yAxisMin = 0;
+  if (yAxisMax === undefined && contributionMode === 'row' && stack) yAxisMax = 1;


This section is fairly opaque and hard to read, wondering if/how much ts can help prevent issues around undef with more types hints here. Nothing outrageous though.

True, will refactor.

mistercrunch · 2020-10-14T04:36:39Z

plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts

-      min: contributionMode === 'row' && stack ? 0 : undefined,
-      max: contributionMode === 'row' && stack ? 1 : undefined,
+      // these might be NaN which break the y axis
+      min: yAxisMin || undefined,


related to the comment above, but what else than int/undef could it be? Also I think 0 would be switched to undef here as 0 || undefined === undefined which seems like it's not desirable if someone wanted 0 to be the bound

villebro · 2020-10-14T06:34:18Z

plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts

+
+  // yAxisBounds sometimes starts returning NaNs, which messes up the u-axis
+  let [min, max] = (yAxisBounds || []).map((val?: number) =>
+    val !== undefined && Number.isNaN(val) ? undefined : val,


Apparently TS doesn't allow for Number.isNaN(undefined), hence the seemingly unnecessary check for undefined here..

You can probably just do yAxisBounds.map(Number) and use Number.isNaN check later.

Thanks, looks much better!

ktmud

LGTM!

villebro · 2020-10-14T07:10:28Z

plugins/plugin-chart-echarts/src/Pie/buildQuery.ts

+  return buildQueryContext(formData, baseQueryObject => [
+    {
+      ...baseQueryObject,
+      orderby: [[metric, false]],


This is now needed to do descending ordering by the chosen metric: apache/superset#11153

feat(plugin-chart-echarts): add truncate y-axis feature

b49ad00

villebro requested a review from a team as a code owner October 13, 2020 08:49

pull-request-size bot added the size/L label Oct 13, 2020

vercel bot deployed to Preview October 13, 2020 08:49 View deployment

villebro changed the title ~~Villebro/yaxis scale~~ fix(plugin-chart-echarts): improve ECharts visuals Oct 13, 2020

villebro force-pushed the villebro/yaxis-scale branch from 1768996 to f089667 Compare October 13, 2020 10:22

vercel bot deployed to Preview October 13, 2020 10:22 View deployment

villebro force-pushed the villebro/yaxis-scale branch from f089667 to 48656f1 Compare October 13, 2020 10:29

vercel bot deployed to Preview October 13, 2020 10:29 View deployment

villebro force-pushed the villebro/yaxis-scale branch from 48656f1 to 5c303a6 Compare October 13, 2020 10:43

vercel bot deployed to Preview October 13, 2020 10:43 View deployment

villebro force-pushed the villebro/yaxis-scale branch from 5c303a6 to 3d4d2c2 Compare October 13, 2020 11:07

vercel bot deployed to Preview October 13, 2020 11:07 View deployment

fix(plugin-chart-echarts): improve ECharts visuals

a38d260

villebro force-pushed the villebro/yaxis-scale branch from 3d4d2c2 to a38d260 Compare October 13, 2020 11:16

vercel bot deployed to Preview October 13, 2020 11:16 View deployment

villebro mentioned this pull request Oct 13, 2020

[Bug] New pie chart labels and control overlap with controls & actual viz apache/superset#11243

Closed

2 tasks

vercel bot deployed to Preview October 13, 2020 21:40 View deployment

add desc ordering

0893d88

villebro force-pushed the villebro/yaxis-scale branch from 6d1cb46 to 0893d88 Compare October 13, 2020 21:49

vercel bot deployed to Preview October 13, 2020 21:49 View deployment

mistercrunch reviewed Oct 14, 2020

View reviewed changes

vercel bot temporarily deployed to Preview October 14, 2020 06:30 Inactive

villebro force-pushed the villebro/yaxis-scale branch from a6c4fad to 49b03dc Compare October 14, 2020 06:33

vercel bot temporarily deployed to Preview October 14, 2020 06:33 Inactive

villebro commented Oct 14, 2020

View reviewed changes

address comments

6e66175

villebro force-pushed the villebro/yaxis-scale branch from 49b03dc to 6e66175 Compare October 14, 2020 06:35

vercel bot deployed to Preview October 14, 2020 06:35 View deployment

ktmud approved these changes Oct 14, 2020

View reviewed changes

simplify isNaN check

674d1d6

vercel bot deployed to Preview October 14, 2020 06:58 View deployment

villebro commented Oct 14, 2020

View reviewed changes

villebro merged commit 23c327c into apache-superset:master Oct 14, 2020

villebro deleted the villebro/yaxis-scale branch October 14, 2020 07:11

villebro mentioned this pull request Oct 14, 2020

feat: add ECharts BoxPlot chart apache/superset#11199

Merged

6 tasks

Conversation

villebro commented Oct 13, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pie

Timeseries

Storybook

Uh oh!

vercel bot commented Oct 13, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Oct 13, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

villebro Oct 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ktmud left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

villebro commented Oct 13, 2020 •

edited

Loading

vercel bot commented Oct 13, 2020 •

edited

Loading

codecov bot commented Oct 13, 2020 •

edited

Loading

villebro Oct 14, 2020 •

edited

Loading