feat(generic-x-axis): add x sorting on series limit metric

[villebro]

SUMMARY

It is often necessary to sort a categorical bar chart by a column that isn't

1. the x-axis
2. the main metric

This is often needed when the underlying categories are ordinal, i.e. have a rank order.

This PR adds the possibility to sort by the series limit metric which is added to the query if it's needed for sorting the query results. To achieve this, one can add a rank ordering dimension to the data and then use that for sorting. In the future we could consider supporting additional metrics in a separate control that could be used to set the color, size or additional context on the tooltip, similarly to how Tableau does it.

SCREENSHOT

See how we are now able to choose the column from the Sort By control:

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

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

[codecov]

Codecov Report

Merging #23274 (94bd97b) into master (42980a6) will decrease coverage by 0.01%.
The diff coverage is 44.82%.

❗ Current head 94bd97b differs from pull request most recent head 6252762. Consider uploading reports for the commit 6252762 to get more accurate results

@@            Coverage Diff             @@
##           master   #23274      +/-   ##
==========================================
- Coverage   67.53%   67.52%   -0.01%     
==========================================
  Files        1907     1908       +1     
  Lines       73465    73483      +18     
  Branches     7976     7985       +9     
==========================================
+ Hits        49612    49622      +10     
- Misses      21801    21808       +7     
- Partials     2052     2053       +1     

Flag	Coverage Δ
javascript	53.77% <44.82%> (+<0.01%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
...et-ui-chart-controls/src/operators/sortOperator.ts	100.00% <ø> (ø)
...rt-controls/src/shared-controls/customControls.tsx	17.39% <0.00%> (-7.61%)	⬇️
.../plugin-chart-echarts/src/Timeseries/buildQuery.ts	70.00% <75.00%> (-1.43%)	⬇️
...t-ui-chart-controls/src/operators/pivotOperator.ts	100.00% <100.00%> (ø)
...ontrols/src/operators/utils/extractExtraMetrics.ts	100.00% <100.00%> (ø)
...gin-chart-echarts/src/Timeseries/transformProps.ts	52.80% <100.00%> (+0.53%)	⬆️
...d/plugins/plugin-chart-echarts/src/utils/series.ts	84.55% <100.00%> (+0.25%)	⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

[kgabryje]

Updating the values in the dropdown seems kinda flaky

Screen.Recording.2023-03-03.at.14.07.59.mov

[kgabryje]

We should figure out a better tooltip for the select - currently it uses the same copy as the checkbox below it

[villebro]

Updating the values in the dropdown seems kinda flaky

Good catch - it seems this shouldMapStateToProps is very flaky, and has been implemented in many places as just () => true. Since this is pretty light weight behavior, I don't see any reason not to do the same here.

[villebro]

We should figure out a better tooltip for the select - currently it uses the same copy as the checkbox below it

Updated + made it flip between X and Y axis like we do in the main "X-Axis" control:

[villebro]

removed due to this hook being flaky (has been disabled on many other similar controls, too)

[kgabryje]

Thanks for the updates! 1 more thing - I think we should display verbose names of columns/metrics in the dropdown

[villebro]

Thanks for the updates! 1 more thing - I think we should display verbose names of columns/metrics in the dropdown

@kgabryje nice catch! Done:

[zhaoyongjie]

LGTM and leave a little unblocked suggestion.

[zhaoyongjie]

I'm thinking whether or not setting extraMetrics into the metrics field in the queryObject, if so might this logic put it in buildQueryObject in the superset-ui/core.

What do you think about this one?

[villebro]

I'm kind of split on this one. I had this going into queryObject originally, but then decided against it, as it would make it ever more difficult to follow which form fields result in which query object fields. I think going forward (v2 chart data API) it would be better to keep the baseQueryObject as slim as possible, and be more explicit about which form data fields should be mapped to which query object fields.

Anyway, I'm not against it, so I'm happy to move this into core later if needed, but for now I think it's simpler to just keep this in chart controls and use it explicitly where it's needed.

[eschutho]

Removing the 2.1.1 labels since this is a feature. We'll get this PR into the 3.0 release.