fix(radar-chart): metric options not available & add min option

[goncaloacteixeira]

SUMMARY

• Advanced metric options for radar chart (defining max values) were not being displayed.
• There was no option to define a min value for each metric, which makes it impossible to have radial lines with the same value for each metric (vertex).

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

Before

After

TESTING INSTRUCTIONS

1. Load the examples
2. Explore a chart of type Radar
3. Under Customize, find the Customize Metrics section, containing the metrics defined for the chart
4. Click on a metric and change the values for Min and Max.

ADDITIONAL INFORMATION

• Has associated issue:
• Required feature flags:
• Changes UI
• Includes DB Migration (follow approval process in SIP-59)
  • Migration is atomic, supports rollback & is backwards-compatible
  • Confirm DB migration upgrade and downgrade tested
  • Runtime estimates and downtime expectations provided
• Introduces new feature or API
• Removes existing feature or API

[github-actions]

Congrats on making your first PR and thank you for contributing to Superset! 🎉 ❤️

We hope to see you in our Slack community too! Not signed up? Use our Slack App to self-register.

[mistercrunch]

does this change the current behavior if the min in the data happens to be negative and no min value is set?

[goncaloacteixeira]

you're right, it shouldn't change the current behaviour, so if the minValueInControl is null it should pass undefined instead of 0

[mistercrunch]

Or maybe you need something similar to how metricValueAsMax is set? idk, depends on what the chart lib is expecting. In any case, you should make sure that it doesn't provoke a change in behavior when not set and that it is the case for when source data contains negative values.

[goncaloacteixeira]

actually, undefined or 0 should result in the same behavior according to the docs:

radar.indicator. min  Try It
number
The minimum value of indicator. It it an optional configuration, with default value of 0.

[goncaloacteixeira]

And this is actually the reason behind this pull request, because we are currently facing an issue with the Radar chart where we have multiple metrics with a range of -4 to 4, and setting the Max as 4 for each metric causes the vertex points to render behind the center point (which I assume is the intended default behavior, since zero is the default value for the minimum).

[goncaloacteixeira]

I think if we want zero as the min, we can either leave it empty, or explicitly define 0. Maybe I'm not understanding your question..

Now that you pointed, the placeholder for the min should be "zero" instead of "auto".

[mistercrunch]

Oh, you wrote:

actually, undefined or 0 should result in the same behavior according to the docs

Which was misleading at first to me assuming their default was, or should be, "the min in the data, even if sub-zero". Given that wrong assumption I thought "but what if you don't want the ACTUAL MIN (assuming actual in was the default) and want zero instead?"

So if you want "zero" you can leave it empty or set it to zero. But what if you want "the min value in the data" which perhaps would be a better default for them (or us) to use ... (?) Say for your use case, you probably want "whatever is the min in the data", which happens to be -4, and it might work to set a hard value in the chart if the data isn't dynamic. Seems that if their default was "the min data from the dataset" you woudn't have had to dig this rabbit hole...

If you felt fancy, you could do look to see if min is undefined on our side, if so you dig in the dataset for the min value and dynamically use that to pass to their library. Essentially if superset's min is undefined we find the min and use that.

[goncaloacteixeira]

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?

[mistercrunch]

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"

[goncaloacteixeira]

I agree! I've updated the logic to align with what you proposed. I don't know if I'm missing something, but I wasn't able to define a default value for the ColumnConfigControl element of the control panel, so instead, I used undefined to identify if the value is not yet set to take the default, zero.

[mistercrunch]

Not sure what's happening with the ci error related to running npm ci, but rebasing the branch might help

[mistercrunch]

Merged, thank you for the contribution! Keep 'em comin'! :)