feat: add Echarts Graph chart #13111

mayurnewase · 2021-02-14T06:48:56Z

SUMMARY

Replace legacy force directed chart with new graph echart
See pr apache-superset/superset-ui#918

ADDITIONAL INFORMATION

codecov-io · 2021-02-14T07:00:01Z

Codecov Report

Merging #13111 (5a2e099) into master (974f447) will decrease coverage by 0.21%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master   #13111      +/-   ##
==========================================
- Coverage   77.14%   76.92%   -0.22%     
==========================================
  Files         865      865              
  Lines       44961    44980      +19     
  Branches     5415     5421       +6     
==========================================
- Hits        34686    34603      -83     
- Misses      10152    10254     +102     
  Partials      123      123

Flag	Coverage Δ
cypress	`58.51% <ø> (-0.03%)`	⬇️
javascript	`62.20% <ø> (+0.02%)`	⬆️
python	`80.39% <ø> (-0.42%)`	⬇️

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

Impacted Files	Coverage Δ
...-frontend/src/visualizations/presets/MainPreset.js	`100.00% <ø> (ø)`
superset/db_engines/hive.py	`0.00% <0.00%> (-85.72%)`	⬇️
superset/db_engine_specs/hive.py	`74.23% <0.00%> (-16.54%)`	⬇️
superset/db_engine_specs/presto.py	`81.62% <0.00%> (-6.42%)`	⬇️
superset/views/database/mixins.py	`81.03% <0.00%> (-1.73%)`	⬇️
superset/db_engine_specs/base.py	`85.57% <0.00%> (-0.49%)`	⬇️
superset/models/core.py	`88.55% <0.00%> (-0.28%)`	⬇️
superset-frontend/src/views/CRUD/types.ts	`100.00% <0.00%> (ø)`
superset/views/core.py	`75.39% <0.00%> (+0.01%)`	⬆️
...-frontend/src/views/CRUD/welcome/ActivityTable.tsx	`72.36% <0.00%> (+6.85%)`	⬆️

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 974f447...5a2e099. Read the comment docs.

ktmud

Since the chart type has been renamed, I'm wondering whether we should add a tooltip (or something else) to help old users find the graph chart option:

But I'm sure people will eventually find it even without this tooltip.

superset/migrations/versions/1412ec1e5a7b_legacy_force_directed_to_echart.py

…d_to_echart.py Co-authored-by: Jesse Yang <jesse.yang@airbnb.com>

junlincc · 2021-02-15T16:10:58Z

@ktmud agreed. I wanted to add tooltips + powered by Echarts to all the new Echarts and reorganize the gallery page.
tagged design-sys-revist. @mayurnewase if it's not much of work, please do add as suggested, we will redesign this page soon.

mayurnewase · 2021-02-15T19:08:54Z

Cool!!!,
How to do it?
Migration to put notification in some table?
@ktmud

ktmud · 2021-02-15T19:24:41Z

Since the chart type has been renamed, I'm wondering whether we should add a tooltip (or something else) to help old users find the graph chart option:

But I'm sure people will eventually find it even without this tooltip.

Cool,how to do it?

Migration to put notification some table?

@ktmud

The tooltip is just a suggestion. We can hard-code a closable one-time tooltip if we want to implement it. It should satisfy following requirements:

It should only display once for users who have not viewed it.
Remember the viewed status in cookie or localStorage.
The tooltip is open by default and the persistent viewed status will only be updated when it is scrolled into viewport.

If this is too much work, feel free to wait for the gallery modal redesign work.

junlincc · 2021-02-20T23:00:24Z

@mayurnewase Mayur, let's not worry about the tooltip for now. Could you get the check pass? thanks!

…or-superset into echart-graph

mayurnewase · 2021-02-21T03:53:54Z

@mayurnewase Mayur, let's not worry about the tooltip for now. Could you get the check pass? thanks!

Sorry didn't see superset-ui release.

ktmud

Also getting this error on db upgrade:

  File "/Users/jesse_yang/opt/anaconda3/envs/superset/lib/python3.7/site-packages/alembic/runtime/environment.py", line 846, in run_migrations
    self.get_context().run_migrations(**kw)
  File "/Users/jesse_yang/opt/anaconda3/envs/superset/lib/python3.7/site-packages/alembic/runtime/migration.py", line 522, in run_migrations
    step.migration_fn(**kw)
  File "/Users/jesse_yang/projects/incubator-superset/superset/migrations/versions/1412ec1e5a7b_legacy_force_directed_to_echart.py", line 64, in upgrade
    del params["collapsed_fieldsets"]
KeyError: 'collapsed_fieldsets'

I tested by creating a new Force Directed graph, change the link length, save it, check out the PR, then run superset db upgrade.

superset-frontend/cypress-base/cypress/integration/explore/visualizations/graph.test.js

superset/migrations/versions/6a8d1a21b532_.py

superset/migrations/versions/1412ec1e5a7b_legacy_force_directed_to_echart.py

mayurnewase · 2021-02-22T14:02:56Z

Hi, @ktmud according to your suggestions for tooltip I have few changes ready locally, which can help adding tooltip to any new chart.
changes below,
master...mayurnewase:chart-tooltip
apache-superset/superset-ui@master...mayurnewase:chart-tooltip

let me know if this can work until this modal gets rewritten, I will clean them up for pr.

Screencast.from.22-02-21.07.47.57.PM.IST.mp4

@junlincc

ktmud · 2021-02-22T19:02:27Z

@mayurnewase Let's save it for another PR. Could you resolve the conflicts so we can merge this PR?

rusackas · 2021-02-22T19:52:51Z

Agreed on merging this PR. I have some thoughts about the tooltip (and related work) but have been pocketing the ideas for a later date. Sneak preview of things I'd like to bring up soon-ish:
• Displaying the tooltip, and having it defined by the plugin as part of a metadata object
• Adding MORE (pre-defined) metadata fields to viz plugins (i.e. tags (realtime support, library used, annotations supported, crossfilters supported, etc)
• Adding the viz category, per SIP-34 or something like it (design needed)
• Adding collapsibles and tag filters to the viz gallery to take advantage of the above metadata
... but all that can wait :)

ktmud · 2021-02-23T02:59:41Z

@rusackas Those are all great ideas! Would love to see them happen.

ktmud · 2021-02-23T04:54:43Z

@mayurnewase your lock file was still using lockfileversion: 1 but we have recently upgraded to npm 7. Please upgrade your node and npm.

I've pushed a new commit to your branch to fix this and update the migration script to always override other params even if groupby or source/target is not present. No action needed if the CI passes.

cc @junlincc if you want to do a testing/product sign-off.

junlincc

Approving as product sign-off.
This is really great, a million thanks for your contribution and collaborating with us! @mayurnewase

My only feedback is. let's remove the directed-force sample chart from the list. do you mind opening another PR? 🙏

ktmud · 2021-02-24T00:57:46Z

It seems there are still directed_force in both test fixtures and example charts. Let's cleaning them all up.

It'd also be great replace directed_force with graph_chart in viz type order list so that Graph Chart can appear in the same place where Force Directed used to be.

junlincc · 2021-02-24T01:07:47Z

@guhan
flagging this PR for risk in db-migration and sample chart/dashboard break.

ktmud · 2021-02-24T01:42:35Z

@junlincc Existing sample chart should not break if you run superset db upgrade, but if we don't fix the test scripts, newly populated sample charts after db migration may break.

villebro · 2021-02-24T06:24:29Z

Let's address the sample charts in a follow up PR

mayurnewase added 2 commits February 14, 2021 11:35

update preset file

ce34834

migration added

72f0f0d

pull-request-size bot added the size/L label Feb 14, 2021

bug fix

2dd58ec

lint

1d54c54

ktmud reviewed Feb 14, 2021

View reviewed changes

superset/migrations/versions/1412ec1e5a7b_legacy_force_directed_to_echart.py Outdated Show resolved Hide resolved

junlincc added viz:chart-forced labels Feb 15, 2021

Update superset/migrations/versions/1412ec1e5a7b_legacy_force_directe…

9451e1f

…d_to_echart.py Co-authored-by: Jesse Yang <jesse.yang@airbnb.com>

mayurnewase added 6 commits February 21, 2021 08:17

version bump

efd8dca

merge master

12826a8

bummp dependency

906cd81

Merge branch 'echart-graph' of https://github.com/mayurnewase/incubat…

d10fdff

…or-superset into echart-graph

add package.lock

ea382ec

merge migrations

4212eea

mayurnewase added 3 commits February 21, 2021 09:29

add integration test

5ab764d

lint

b6129b4

remove unnecessary params from test

2d0a955

ktmud added the risk:db-migration PRs that require a DB migration label Feb 21, 2021

ktmud reviewed Feb 21, 2021

View reviewed changes

superset-frontend/cypress-base/cypress/integration/explore/visualizations/graph.test.js Outdated Show resolved Hide resolved

superset/migrations/versions/6a8d1a21b532_.py Outdated Show resolved Hide resolved

ktmud reviewed Feb 21, 2021

View reviewed changes

superset/migrations/versions/1412ec1e5a7b_legacy_force_directed_to_echart.py Show resolved Hide resolved

mayurnewase added 3 commits February 21, 2021 19:01

check key existance in migrations

0480b23

test to typescript

127aae7

Update graph.test.ts

59ad92e

fix migration downstream head

ac274a6

mayurnewase requested a review from ktmud February 22, 2021 02:37

merge with master

3a19642

Upgrade to lockfileversion 2

5a2e099

ktmud force-pushed the echart-graph branch from a03a032 to 5a2e099 Compare February 23, 2021 04:53

ktmud approved these changes Feb 23, 2021

View reviewed changes

villebro approved these changes Feb 23, 2021

View reviewed changes

junlincc self-requested a review February 24, 2021 00:41

junlincc approved these changes Feb 24, 2021

View reviewed changes

junlincc merged commit 6954114 into apache:master Feb 24, 2021

mayurnewase mentioned this pull request Mar 3, 2021

chore: clean remaining old directed force graph code #13431

Merged

6 tasks

junlincc mentioned this pull request Mar 6, 2021

Energy Force chart showing an error on the Misc Charts example dashboard #13494

Closed

3 tasks

rusackas mentioned this pull request Jan 27, 2023

[Graph]color scheme change does not take effect #16217

Closed

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add Echarts Graph chart #13111

feat: add Echarts Graph chart #13111

mayurnewase commented Feb 14, 2021 •

edited

Loading

codecov-io commented Feb 14, 2021 •

edited

Loading

ktmud left a comment

junlincc commented Feb 15, 2021

mayurnewase commented Feb 15, 2021 •

edited

Loading

ktmud commented Feb 15, 2021 •

edited

Loading

junlincc commented Feb 20, 2021

mayurnewase commented Feb 21, 2021

ktmud left a comment

mayurnewase commented Feb 22, 2021 •

edited

Loading

ktmud commented Feb 22, 2021

rusackas commented Feb 22, 2021 •

edited

Loading

ktmud commented Feb 23, 2021

ktmud commented Feb 23, 2021 •

edited

Loading

junlincc left a comment •

edited

Loading

ktmud commented Feb 24, 2021 •

edited

Loading

junlincc commented Feb 24, 2021

ktmud commented Feb 24, 2021

villebro commented Feb 24, 2021

feat: add Echarts Graph chart #13111

feat: add Echarts Graph chart #13111

Conversation

mayurnewase commented Feb 14, 2021 • edited Loading

SUMMARY

ADDITIONAL INFORMATION

codecov-io commented Feb 14, 2021 • edited Loading

Codecov Report

ktmud left a comment

Choose a reason for hiding this comment

junlincc commented Feb 15, 2021

mayurnewase commented Feb 15, 2021 • edited Loading

ktmud commented Feb 15, 2021 • edited Loading

junlincc commented Feb 20, 2021

mayurnewase commented Feb 21, 2021

ktmud left a comment

Choose a reason for hiding this comment

mayurnewase commented Feb 22, 2021 • edited Loading

ktmud commented Feb 22, 2021

rusackas commented Feb 22, 2021 • edited Loading

ktmud commented Feb 23, 2021

ktmud commented Feb 23, 2021 • edited Loading

junlincc left a comment • edited Loading

Choose a reason for hiding this comment

ktmud commented Feb 24, 2021 • edited Loading

junlincc commented Feb 24, 2021

ktmud commented Feb 24, 2021

villebro commented Feb 24, 2021

mayurnewase commented Feb 14, 2021 •

edited

Loading

codecov-io commented Feb 14, 2021 •

edited

Loading

mayurnewase commented Feb 15, 2021 •

edited

Loading

ktmud commented Feb 15, 2021 •

edited

Loading

mayurnewase commented Feb 22, 2021 •

edited

Loading

rusackas commented Feb 22, 2021 •

edited

Loading

ktmud commented Feb 23, 2021 •

edited

Loading

junlincc left a comment •

edited

Loading

ktmud commented Feb 24, 2021 •

edited

Loading