goncaloacteixeira commented on code in PR #30349:
URL: https://github.com/apache/superset/pull/30349#discussion_r1769368530
##########
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 totally agree! For my, personally, it's visually hard to interpret a
vertex that goes the other way from the center point, even more so once we
increase the number of dimensions.
So, while the library's default is "zero" when it's undefined, I agree it
would benefit the users that if the superset min is left undefined, then we
should look for the minimum value in the dataset.
But this is where things get tricky, while for a negative range this makes
sense (since we don't want the points to go over to the other side), if we have
a positive-only range, the minimum value should be zero, right?
##########
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 totally agree! For me, personally, it's visually hard to interpret a
vertex that goes the other way from the center point, even more so once we
increase the number of dimensions.
So, while the library's default is "zero" when it's undefined, I agree it
would benefit the users that if the superset min is left undefined, then we
should look for the minimum value in the dataset.
But this is where things get tricky, while for a negative range this makes
sense (since we don't want the points to go over to the other side), if we have
a positive-only range, the minimum value should be zero, right?
--
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]
