feat: Adds the ECharts Bubble chart #22107

mayurnewase · 2022-11-13T09:28:31Z

SUMMARY

Adds the ECharts Bubble chart.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

Chart and data control panel:
Customize control panel
Tooltip:

TESTING INSTRUCTIONS

create d3 bubble chart
merge commits from this PR and open existing chart should work fine as before.
test all controls in data and customize sections.

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 · 2022-11-23T05:23:00Z

Codecov Report

Merging #22107 (df22d86) into master (389e44e) will increase coverage by 0.26%.
The diff coverage is 74.46%.

@@            Coverage Diff             @@
##           master   #22107      +/-   ##
==========================================
+ Coverage   66.65%   66.91%   +0.26%     
==========================================
  Files        1841     1852      +11     
  Lines       70220    70316      +96     
  Branches     7670     7684      +14     
==========================================
+ Hits        46804    47052     +248     
+ Misses      21434    21268     -166     
- Partials     1982     1996      +14

Flag	Coverage Δ
hive	`52.55% <ø> (?)`
javascript	`53.84% <74.46%> (+0.18%)`	⬆️
mysql	`78.04% <ø> (-0.03%)`	⬇️
postgres	`78.11% <ø> (-0.03%)`	⬇️
presto	`52.45% <ø> (?)`
python	`81.30% <ø> (+0.36%)`	⬆️
sqlite	`76.57% <ø> (?)`
unit	`50.86% <ø> (-0.01%)`	⬇️

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

Impacted Files	Coverage Δ
.../plugin-chart-echarts/src/Bubble/EchartsBubble.tsx	`0.00% <0.00%> (ø)`
...ntend/plugins/plugin-chart-echarts/src/defaults.ts	`100.00% <ø> (+80.00%)`	⬆️
...-frontend/src/visualizations/presets/MainPreset.js	`100.00% <ø> (ø)`
...s/plugin-chart-echarts/src/Bubble/controlPanel.tsx	`50.00% <50.00%> (ø)`
...d/plugins/plugin-chart-echarts/src/Bubble/index.ts	`50.00% <50.00%> (ø)`
...gins/plugin-chart-echarts/src/Bubble/buildQuery.ts	`66.66% <66.66%> (ø)`
.../plugin-chart-echarts/src/Bubble/transformProps.ts	`82.35% <82.35%> (ø)`
...ugins/plugin-chart-echarts/src/Bubble/constants.ts	`100.00% <100.00%> (ø)`
...ugins/legacy-preset-chart-nvd3/src/Bubble/index.js	`33.33% <0.00%> (-33.34%)`	⬇️
...c/components/Chart/DrillDetail/DrillDetailPane.tsx	`76.04% <0.00%> (-6.68%)`	⬇️
... and 31 more

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

mayurnewase · 2022-11-23T06:10:07Z

superset-frontend/plugins/plugin-chart-echarts/src/Bubble/transformProps.ts

+    ? `${params.data[3]} <sub>(${params.data[4]})</sub>`
+    : params.data[3];
+
+  return `<p>${title}</p>


didn't use echart's sanitize function, it removes line breaks.

superset-frontend/.storybook/main.js

superset-frontend/plugins/plugin-chart-echarts/src/Bubble/index.ts

superset-frontend/plugins/plugin-chart-echarts/src/Bubble/transformProps.ts

villebro

First pass comments, looks great! One thing that I noticed is that the NVD3 plugin assumes truncation on the y-axis if the bounds aren't set. While I don't necessarily like the idea of defaulting to truncation (actually, I strongly oppose it), I was just wondering if we should do a migration that enables truncation for existing charts if the bounds are undefined?

superset-frontend/plugins/plugin-chart-echarts/src/Bubble/transformProps.ts

mayurnewase · 2022-11-29T14:05:22Z

First pass comments, looks great! One thing that I noticed is that the NVD3 plugin assumes truncation on the y-axis if the bounds aren't set. While I don't necessarily like the idea of defaulting to truncation (actually, I strongly oppose it), I was just wondering if we should do a migration that enables truncation for existing charts if the bounds are undefined?

agree, added migration.

villebro

A few more things that came to mind. Also, one major issue: the chart is heavily padded:

I quickly verified that by adding left: 0, right: 0 to the grid seemed to widen it to the full width, although it went a few pixels over. So I think we need to reuse the getPadding util from the Timeseries chart to get the grid right.

superset-frontend/plugins/plugin-chart-echarts/src/Bubble/transformProps.ts

zhaoyongjie

I just saw the original bubble chart(NVD3 version), and the limit control have an inconsistent setting. IMO, we should fix/improve that in the new version of Bubble Chart.

The Series Limit refers to a limit by series rather than a row limit.
No chance to set a order by clause in the SQL

superset-frontend/plugins/plugin-chart-echarts/src/Bubble/buildQuery.ts

superset-frontend/plugins/plugin-chart-echarts/src/Bubble/constants.ts

mayurnewase · 2022-11-30T07:47:45Z

I just saw the original bubble chart(NVD3 version), and the limit control have an inconsistent setting. IMO, we should fix/improve that in the new version of Bubble Chart.

The Series Limit refers to a limit by series rather than a row limit.

No chance to set a order by clause in the SQL

here series limit made sense as its inline with echart's options.(every bubble is separate series)
and description of dimension in control panel had series in it to group them by color.

zhaoyongjie · 2022-11-30T08:01:08Z

@mayurnewase the series limit will send a subquery instead a simple query, the time-series chart should be referenced.

mayurnewase · 2022-11-30T08:27:14Z

@mayurnewase the series limit will send a subquery instead a simple query, the time-series chart should be referenced.

I see, let me check.

villebro · 2022-11-30T10:44:43Z

FYI @mayurnewase @zhaoyongjie , slightly related, I'm opening a PR today that will clean up deprecated limit control logic from superset-ui/core so we can remove those annoying deprecation warnings on chart data requests

mayurnewase · 2022-12-01T09:43:56Z

updated series limit to row limit, and added order control.

@zhaoyongjie the normalizeOrderBy in core doesn't support this control panel yet and I didn't want to change anything in core in this PR so added order support in the buildQuery itself.
Either this or need to something more hacky like below

orderby: normalizeOrderBy( { ...baseQueryObject, orderby: [ [...baseQueryObject.orderby, !baseQueryObject.order_desc] ] } ).orderby

what do you think?

…tters

michael-s-molina · 2023-09-19T21:36:02Z

Hi @mayurnewase. Thank you for this contribution. I would like to continue the work and see if we can ship this feature. To do that I can push to your PR if you give me the GitHub permission or I can close this PR, open my own and reference you as the original author. Which method do you prefer?

mayurnewase · 2023-09-19T22:08:41Z

Hey thanks for taking this up, I am ok with both methods.
Added you as a collaborator in my fork of this repo, and I see Maintainers are allowed to edit this pull request. in the right panel.

michael-s-molina · 2023-09-20T11:14:47Z

Added you as a collaborator in my fork of this repo, and I see Maintainers are allowed to edit this pull request. in the right panel.

Thanks @mayurnewase. I'll rebase the PR and modify its scope a little bit to avoid introducing a breaking change. I'll move the migration script to its own PR, similar to #23973 and also keep the legacy plugin. The new scope will be to just add the ECharts Bubble chart which will allow us to merge this PR without the need to wait for a major version release.

michael-s-molina · 2023-09-20T16:43:14Z

I deprecated the legacy chart and added thumbnails for the new version.

michael-s-molina · 2023-09-20T17:59:55Z

/testenv up

github-actions · 2023-09-20T18:01:55Z

@michael-s-molina Ephemeral environment spinning up at http://34.217.102.24:8080. Credentials are admin/admin. Please allow several minutes for bootstrapping and startup.

villebro

I think this looks great. The only nit that comes to mind is different handling of the title and missing colors in the tooltip:

legacy:

new:

But as discussed a few months ago, I think we have some tooltip harmonization and re-styling work to do anyway, so maybe this can be deferred to that effort.

michael-s-molina · 2023-10-05T10:58:37Z

Thanks for the review @villebro!

But as discussed a few months ago, I think we have some tooltip harmonization and re-styling work to do anyway, so maybe this can be deferred to that effort.

Yes, let's tackle this in a follow-up as part of the harmonization and re-styling work.

github-actions · 2023-10-05T10:59:18Z

Ephemeral environment shutdown and build artifacts deleted.

Co-authored-by: Michael S. Molina <michael.s.molina@gmail.com>

pull-request-size bot added the size/L label Nov 13, 2022

mayurnewase changed the title ~~[WIP] feat: migrate bubble char to echarts~~ [WIP] feat: migrate bubble chart to echarts Nov 13, 2022

pull-request-size bot added size/XL and removed size/L labels Nov 23, 2022

mayurnewase marked this pull request as ready for review November 23, 2022 05:17

mayurnewase changed the title ~~[WIP] feat: migrate bubble chart to echarts~~ feat: migrate bubble chart to echarts Nov 23, 2022

pull-request-size bot added size/XXL and removed size/XL labels Nov 23, 2022

mayurnewase commented Nov 23, 2022

View reviewed changes

superset-frontend/.storybook/main.js Outdated Show resolved Hide resolved

mayurnewase commented Nov 23, 2022

View reviewed changes

superset-frontend/plugins/plugin-chart-echarts/src/Bubble/index.ts Outdated Show resolved Hide resolved

mayurnewase commented Nov 23, 2022

View reviewed changes

superset-frontend/plugins/plugin-chart-echarts/src/Bubble/transformProps.ts Show resolved Hide resolved

villebro reviewed Nov 28, 2022

View reviewed changes

mayurnewase requested review from jinghua-qa, geido and a team as code owners November 29, 2022 07:30

mayurnewase requested a review from villebro November 29, 2022 14:05

villebro reviewed Nov 29, 2022

View reviewed changes

superset-frontend/plugins/plugin-chart-echarts/src/Bubble/transformProps.ts Show resolved Hide resolved

superset-frontend/plugins/plugin-chart-echarts/src/Bubble/transformProps.ts Outdated Show resolved Hide resolved

mayurnewase force-pushed the feat/echart-bubble-chart branch from ee9ac71 to 6d6c822 Compare November 30, 2022 06:51

mayurnewase commented Nov 30, 2022

View reviewed changes

superset-frontend/plugins/plugin-chart-echarts/src/Bubble/transformProps.ts Outdated Show resolved Hide resolved

mayurnewase requested a review from villebro November 30, 2022 07:02

zhaoyongjie reviewed Nov 30, 2022

View reviewed changes

superset-frontend/plugins/plugin-chart-echarts/src/Bubble/buildQuery.ts Show resolved Hide resolved

zhaoyongjie reviewed Nov 30, 2022

View reviewed changes

superset-frontend/plugins/plugin-chart-echarts/src/Bubble/constants.ts Outdated Show resolved Hide resolved

mayurnewase added 4 commits September 19, 2023 18:24

fix padding, add refs, improve bubble label test with different forma…

4ff0d99

…tters

remove redundant description from controls

8be37ea

update buildquery for columns, show dimension in tooltip on next line

15fb7d6

add orderby support, fix log on axes

adc4848

michael-s-molina force-pushed the feat/echart-bubble-chart branch from df22d86 to adc4848 Compare September 20, 2023 11:05

michael-s-molina requested review from eschutho, rusackas and betodealmeida as code owners September 20, 2023 11:05

michael-s-molina changed the title ~~feat: migrate bubble chart to echarts~~ feat: Adds the ECharts Bubble chart Sep 20, 2023

michael-s-molina marked this pull request as draft September 20, 2023 11:16

michael-s-molina added 5 commits September 20, 2023 08:19

Removes migration script

7440f28

Preserves legacy plugin

c1ff00b

Adds thumbnails

90f157e

Fixes lint errors

452a5b3

Changes default Y-axis margin

53c2cc4

michael-s-molina marked this pull request as ready for review September 20, 2023 16:40

michael-s-molina requested a review from kgabryje September 20, 2023 16:47

villebro approved these changes Oct 4, 2023

View reviewed changes

michael-s-molina merged commit c81c60c into apache:master Oct 5, 2023
46 checks passed

cccs-rc pushed a commit to CybercentreCanada/superset that referenced this pull request Mar 6, 2024

feat: Adds the ECharts Bubble chart (apache#22107)

e097e4d

Co-authored-by: Michael S. Molina <michael.s.molina@gmail.com>

mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 3.1.0 labels Mar 13, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Adds the ECharts Bubble chart #22107

feat: Adds the ECharts Bubble chart #22107

mayurnewase commented Nov 13, 2022 •

edited by michael-s-molina

Loading

codecov bot commented Nov 23, 2022 •

edited

Loading

mayurnewase Nov 23, 2022

villebro left a comment

mayurnewase commented Nov 29, 2022

villebro left a comment

zhaoyongjie left a comment •

edited

Loading

mayurnewase commented Nov 30, 2022

zhaoyongjie commented Nov 30, 2022 •

edited

Loading

mayurnewase commented Nov 30, 2022

villebro commented Nov 30, 2022

mayurnewase commented Dec 1, 2022

michael-s-molina commented Sep 19, 2023 •

edited

Loading

mayurnewase commented Sep 19, 2023

michael-s-molina commented Sep 20, 2023

michael-s-molina commented Sep 20, 2023

michael-s-molina commented Sep 20, 2023

github-actions bot commented Sep 20, 2023

villebro left a comment •

edited

Loading

michael-s-molina commented Oct 5, 2023

github-actions bot commented Oct 5, 2023

feat: Adds the ECharts Bubble chart #22107

feat: Adds the ECharts Bubble chart #22107

Conversation

mayurnewase commented Nov 13, 2022 • edited by michael-s-molina Loading

SUMMARY

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

codecov bot commented Nov 23, 2022 • edited Loading

Codecov Report

mayurnewase Nov 23, 2022

Choose a reason for hiding this comment

villebro left a comment

Choose a reason for hiding this comment

mayurnewase commented Nov 29, 2022

villebro left a comment

Choose a reason for hiding this comment

zhaoyongjie left a comment • edited Loading

Choose a reason for hiding this comment

mayurnewase commented Nov 30, 2022

zhaoyongjie commented Nov 30, 2022 • edited Loading

mayurnewase commented Nov 30, 2022

villebro commented Nov 30, 2022

mayurnewase commented Dec 1, 2022

michael-s-molina commented Sep 19, 2023 • edited Loading

mayurnewase commented Sep 19, 2023

michael-s-molina commented Sep 20, 2023

michael-s-molina commented Sep 20, 2023

michael-s-molina commented Sep 20, 2023

github-actions bot commented Sep 20, 2023

villebro left a comment • edited Loading

Choose a reason for hiding this comment

michael-s-molina commented Oct 5, 2023

github-actions bot commented Oct 5, 2023

mayurnewase commented Nov 13, 2022 •

edited by michael-s-molina

Loading

codecov bot commented Nov 23, 2022 •

edited

Loading

zhaoyongjie left a comment •

edited

Loading

zhaoyongjie commented Nov 30, 2022 •

edited

Loading

michael-s-molina commented Sep 19, 2023 •

edited

Loading

villebro left a comment •

edited

Loading