Skip to content
This repository has been archived by the owner on Dec 10, 2021. It is now read-only.

fix(plugin-chart-echarts): remove label line if below threshold #1071

Merged
merged 1 commit into from
Apr 26, 2021

Conversation

villebro
Copy link
Contributor

🐛 Bug Fix

When showLabelsThreshold is set, the label lines are shown despite the labels being missing. This PR removes the hack to make labels empty strings when the threshold is not met and implements the minShowLabelAngle (set to 3.6 * showLabelsThreshold as 100 % == 360 degrees) property that provides native support for removal of thin slices.

BEFORE

As can be seen in the screenshot, the top slices show a line despite no labels being shown:
image

AFTER

After the fix, no label lines are shown for the slices that don't exceed the threshold:
image

@villebro villebro requested a review from a team as a code owner April 24, 2021 06:14
@vercel
Copy link

vercel bot commented Apr 24, 2021

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/56DeUX3hHWPC8bobiNtMZRudQsgX
✅ Preview: https://superset-ui-git-fork-preset-io-villebro-pie-min-angle-superset.vercel.app

@@ -102,6 +102,7 @@ export default function transformProps(chartProps: EchartsPieChartProps): PieCha
emitFilter,
}: EchartsPieFormData = { ...DEFAULT_LEGEND_FORM_DATA, ...DEFAULT_PIE_FORM_DATA, ...formData };
const metricLabel = getMetricLabel(metric);
const minShowLabelAngle = (showLabelsThreshold || 0) * 3.6;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It turns out the TextControl with isFloat: true returns an empty string when unset, making it important to replace a falsy value for showLabelsThreshold with zero.

@codecov
Copy link

codecov bot commented Apr 24, 2021

Codecov Report

Merging #1071 (8ee2640) into master (ec94457) will not change coverage.
The diff coverage is 33.33%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1071   +/-   ##
=======================================
  Coverage   27.80%   27.80%           
=======================================
  Files         453      453           
  Lines        9104     9104           
  Branches     1416     1416           
=======================================
  Hits         2531     2531           
+ Misses       6382     6381    -1     
- Partials      191      192    +1     
Impacted Files Coverage Δ
...ins/plugin-chart-echarts/src/Pie/transformProps.ts 70.45% <33.33%> (ø)

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 ec94457...8ee2640. Read the comment docs.

Copy link
Contributor

@zhaoyongjie zhaoyongjie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Thank you for this fix.

@villebro villebro merged commit 158a777 into apache-superset:master Apr 26, 2021
@villebro villebro deleted the villebro/pie-min-angle branch April 26, 2021 08:48
@junlincc
Copy link
Contributor

Thank you @villebro , i believe the actual implementation/fix is happening in Echarts repo and will be available in their next release?

@villebro
Copy link
Contributor Author

@junlincc you are right - this fix only partly works on the 5.1.0 release - the rest of it is implemented in 5.1.1 (apache/echarts#14702) which is being bumped here: #1074

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants