mistercrunch commented on code in PR #30349:
URL: https://github.com/apache/superset/pull/30349#discussion_r1769651182
##########
superset-frontend/plugins/plugin-chart-echarts/src/Radar/transformProps.ts:
##########
@@ -199,16 +199,20 @@ export default function transformProps(
const indicator = metricLabels.map(metricLabel => {
const maxValueInControl = columnConfig?.[metricLabel]?.radarMetricMaxValue;
+ const minValueInControl = columnConfig?.[metricLabel]?.radarMetricMinValue;
+
// Ensure that 0 is at the center of the polar coordinates
const metricValueAsMax =
metricLabelAndMaxValueMap.get(metricLabel) === 0
? Number.MAX_SAFE_INTEGER
: metricLabelAndMaxValueMap.get(metricLabel);
const max =
maxValueInControl === null ? metricValueAsMax : maxValueInControl;
+ const min = minValueInControl === null ? 0 : minValueInControl;
Review Comment:
I have to admit I haven't used these spider web charts often, and it's hard
for me to think of a legit use case with negative values. Actually here's one,
let's say you'd want to plot different products, and one metric is profit/loss.
Say the worst product is -100k and best one is positive 200k. I think you'd
just want for the linear scale to start at -100k as the center point and go
outwards from there. Similarly, something like "goal achievement" which could
come short and spawn across negative/positive. Seems in both cases you'd want
the min value.
Now take something like "revenue" or anything else that can only be
positive. I think much like bar chart, having a dynamic axis for those is
actually bad. Say if the values are 100, 150 and 200, you want for 200 to look
twice as good as 100. If you go dynamic on those (with the axis starting at 100
instead of zero), then 200 looks twice as good as 150, which isn't right.
This means that zero is a fairly sensible default for metrics that don't
include negatives.
Another note, from your screenshot, empty placeholder says "auto", which
seems misleading as `undefined` really means zero, which is a sensible choice
for positive-only metrics.
Oh also, one question, what does the chart lib do with negative values when
min is set to `0` today? Does it plot the dot across the mid point? Does that
make any sense visually?
Alright, so what does all this inform?
My vote would be to:
- instead of using "empty" (meaning "auto") as a default on the Superset
form side, we put a hard `0` in there on the `min` size, and keep `auto` on the
max side. Meaning when you open that popover for the first time, it's set to
mix=0 and max=auto
- if someone REMOVES that zero to effectively choose `auto`, because either
they want to set a new baseline for the center point, or have negative values,
then we look for the min data point to set the min. So "auto" really means "set
the scale based on the data dynamically"
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
