Invalid behavior of funnel chart sorting

dimoha commented 9 months ago

Bug description

Hello. I would like to report that I found a mistake in Funnel chart.

The fact is that the SORT BY METRIC option does not actually affect the order in which the layers are displayed in the funnel, since the sorting of these layers depends on the “sort” setting in the plugin. You can check and make sure that it doesn't matter whether this option is enabled or not - the layers will not change position relative to each other. Therefore, it seems to me that the right solution is to remove the SORT BY METRIC option from the DATA section, and in the CUSTOMIZE section create a sort option that will take one of three values: 'ascending', 'descending', 'none' As stated in the specification https://echarts.apache.org/en/option.html#series-funnel.sort The default value should be set to "descending"

How to reproduce the bug

Create a funnel chart
Try change SORT BY METRIC

Screenshots/recordings

No response

Superset version

master / latest-dev

Python version

3.9

Node version

16

Browser

Chrome

Additional context

No response

Checklist

[x] I have searched Superset docs and Slack and didn't find a solution to my problem.
[x] I have searched the GitHub issue tracker and didn't find a similar bug report.
[ ] I have checked Superset's logs for errors and if I found a relevant Python stacktrace, I included it here as text in the "additional context" section.

rusackas commented 4 months ago

I believe the Sort By Metric sorts the results generated by the query, does it not? Adding the control for the display would also be a nice addition, but these seem like different controls for different purposes.

Rusp0 commented 2 months ago

In addition to this, I would like to say that if the sorting is by value, then I would like to be able to change the order of the funnel steps, since with equal values, logically, the steps should be reversed in some cases

dimoha commented 3 weeks ago

I believe the Sort By Metric sorts the results generated by the query, does it not? Adding the control for the display would also be a nice addition, but these seem like different controls for different purposes.

you are right

apache / superset