Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: Bar stacking on v2 doesn't work if the x-axis is value type #20591

Conversation

diegomedina248
Copy link
Contributor

SUMMARY

When GENERIC_CHART_AXES is enabled, stacking on bar chart v2 is not working correctly in all cases.
The reason is that bar stacking is supported by echarts only for categorical types.
Source: apache/echarts#15102

This PR hides that feature if the axis is not categorical.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

Before:

Screen.Recording.2022-07-01.at.15.06.09.mov

After:

new.mov

TESTING INSTRUCTIONS

  1. Enable GENERIC_CHART_AXES.
  2. Create Bar v2 charts with different types in the x-types.

Ensure the stack option is only available for categorical types, and that it works as expected.

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

@diegomedina248 diegomedina248 force-pushed the fix/echarts-bar-stacking-value-type branch from b6e16a2 to cfb8974 Compare July 2, 2022 00:06
@codecov
Copy link

codecov bot commented Jul 2, 2022

Codecov Report

Merging #20591 (cfb8974) into master (b870a21) will decrease coverage by 0.00%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##           master   #20591      +/-   ##
==========================================
- Coverage   66.79%   66.78%   -0.01%     
==========================================
  Files        1754     1754              
  Lines       65561    65575      +14     
  Branches     6933     6939       +6     
==========================================
+ Hits        43793    43796       +3     
- Misses      20014    20024      +10     
- Partials     1754     1755       +1     
Flag Coverage Δ
javascript 51.80% <0.00%> (-0.02%) ⬇️

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

Impacted Files Coverage Δ
...d/packages/superset-ui-chart-controls/src/types.ts 100.00% <ø> (ø)
...tend/plugins/plugin-chart-echarts/src/controls.tsx 45.71% <0.00%> (-27.02%) ⬇️
...c/filters/components/Select/SelectFilterPlugin.tsx 62.82% <0.00%> (-1.29%) ⬇️
superset-frontend/src/components/Select/Select.tsx 87.44% <0.00%> (ø)
...set-ui-core/src/ui-overrides/UiOverrideRegistry.ts 100.00% <0.00%> (ø)
...d/src/SqlLab/components/SaveDatasetModal/index.tsx 34.61% <0.00%> (+0.46%) ⬆️
...tend/src/views/CRUD/data/database/DatabaseList.tsx 68.36% <0.00%> (+0.65%) ⬆️
...perset-frontend/src/views/CRUD/welcome/Welcome.tsx 75.00% <0.00%> (+0.77%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b870a21...cfb8974. Read the comment docs.

@zhaoyongjie
Copy link
Member

/testenv up

@github-actions
Copy link
Contributor

github-actions bot commented Jul 4, 2022

@zhaoyongjie Ephemeral environment spinning up at http://54.184.228.100:8080. Credentials are admin/admin. Please allow several minutes for bootstrapping and startup.

@zhaoyongjie
Copy link
Member

/testenv up FEATURE_GENERIC_CHART_AXES=true

@github-actions
Copy link
Contributor

github-actions bot commented Jul 4, 2022

@zhaoyongjie Ephemeral environment spinning up at http://35.89.160.47:8080. Credentials are admin/admin. Please allow several minutes for bootstrapping and startup.

@zhaoyongjie zhaoyongjie self-requested a review July 4, 2022 03:32
Copy link
Member

@zhaoyongjie zhaoyongjie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The root cause of the case was multiple metrics applied in the stack bar chart.

  1. The bar chart in the Echart supports timeseries as the x-axis to stack. e.g.
barchart.timeseries.stack.mov
  1. we should find a way to support multiple metrics in the bar chart, or just avoid applying multiple metrics in the stacked bar chart, instead of removing the entire stack feature in the timeseries barchart.

@rusackas
Copy link
Member

@diegomedina248 @zhaoyongjie should we set up a time to sync on this and get to the bottom of it?

@diegomedina248
Copy link
Contributor Author

Closed in favor on #20805

@diegomedina248 diegomedina248 deleted the fix/echarts-bar-stacking-value-type branch August 10, 2022 15:35
@github-actions
Copy link
Contributor

Ephemeral environment shutdown and build artifacts deleted.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants