Skip to content
This repository has been archived by the owner on Dec 10, 2021. It is now read-only.

feat(plugin-chart-echarts): add graph echart #918

Merged
merged 59 commits into from
Feb 19, 2021

Conversation

mayurnewase
Copy link
Contributor

@mayurnewase mayurnewase commented Jan 27, 2021

🏆 Enhancements
Migrated legacy force directed chart to echart

Todos

  • Optional category option
  • Color from control panel
  • Show tooltip on hover
  • Cleanups + Prettify
  • Add thumbnail
  • Few typing issues
  • Legend customization
  • Tests

Screenshots
1.Circular type
Screenshot from 2021-02-06 20-27-35

2.Force type
Screenshot from 2021-02-06 20-29-50

remaining:
	expose those in control panel
	make them all work
	add tests
@mayurnewase mayurnewase requested a review from a team as a code owner January 27, 2021 06:31
@vercel
Copy link

vercel bot commented Jan 27, 2021

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/superset/superset-ui-mcn5lxgmc/superset
✅ Preview: https://superset-ui-git-fork-mayurnewase-echart-force-directed.superset.vercel.app

@mayurnewase mayurnewase changed the title Echart force directed feat(plugin-chart-echarts): add graph echart Jan 27, 2021
@mayurnewase
Copy link
Contributor Author

@villebro

@villebro
Copy link
Contributor

This is wonderful @mayurnewase !!!!! 🎉 Really looking forward to testing + reviewing!

@mayurnewase mayurnewase changed the title feat(plugin-chart-echarts): add graph echart [WIP] feat(plugin-chart-echarts): add graph echart Jan 27, 2021
@mayurnewase mayurnewase changed the title [WIP] feat(plugin-chart-echarts): add graph echart [WIP]feat(plugin-chart-echarts): add graph echart Jan 27, 2021
@mayurnewase
Copy link
Contributor Author

mayurnewase commented Jan 31, 2021

@villebro Should we seperate groupby field into source and target?
Screenshot_2021-01-31  DEV  FCC Chart Graph

@junlincc
Copy link
Contributor

@villebro Should we seperate groupby field like source and target?
Screenshot_2021-01-31 DEV FCC Chart Graph

Yes, please do. This has caused major confusion. @mayurnewase

@villebro
Copy link
Contributor

@mayurnewase yes, I'm not sure why it's been designed that way, but having separate single selects for source and target makes much more sense 👍

Copy link
Contributor

@ktmud ktmud left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution. Adding a couple of comments on code style and typing.

+1 on separating source and target control.

plugins/plugin-chart-echarts/src/Pie/transformProps.ts Outdated Show resolved Hide resolved
plugins/plugin-chart-echarts/src/index.ts Outdated Show resolved Hide resolved
plugins/plugin-chart-echarts/src/Graph/transformProps.ts Outdated Show resolved Hide resolved
plugins/plugin-chart-echarts/src/Graph/index.ts Outdated Show resolved Hide resolved
plugins/plugin-chart-echarts/src/Graph/controlPanel.tsx Outdated Show resolved Hide resolved
plugins/plugin-chart-echarts/src/Graph/controlPanel.tsx Outdated Show resolved Hide resolved
plugins/plugin-chart-echarts/src/Graph/controlPanel.tsx Outdated Show resolved Hide resolved
plugins/plugin-chart-echarts/src/Graph/types.ts Outdated Show resolved Hide resolved
plugins/plugin-chart-echarts/src/Graph/types.ts Outdated Show resolved Hide resolved
@rusackas
Copy link
Member

rusackas commented Feb 2, 2021

Very excited about this!

One small detail before this gets merged is to update the thumbnail to include a pic of the force graph itself :)

Copy link
Contributor

@villebro villebro left a comment

Choose a reason for hiding this comment

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

This is super nice! 🎉 One last recommendation for reducing default zoom level, other than that this seems good to go after the linting errors are fixed!

import { GraphSeriesOption } from 'echarts';

export const DEFAULT_GRAPH_SERIES_OPTION: GraphSeriesOption = {
zoom: 1,
Copy link
Contributor

Choose a reason for hiding this comment

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

I noticed that this might be better at something lower; see the 0.7 (upper) vs the current default of 1.0 (lower). This applies especially to circular mode, but it appears to also be over-zoomed on force mode.
image
image

[<h1 className="section-header">{t('Legend')}</h1>],
[showLegendControl],
[legendTypeControl, legendOrientationControl],
[legendMarginControl, noopControl],
Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, it appears to be unrelated to this PR, so better save for another PR

@ktmud
Copy link
Contributor

ktmud commented Feb 17, 2021

@mayurnewase I did some further optimization and pushed directly to your branch. Hope you don't mind.

Here's a summary of the changes:

  1. Refactored the loops and added normalized edges

  2. Added separate controls for source node category and target node category.
    graph-category

  3. Merged setLabelVisibility and setNormalizedSymbolSize to one loop

  4. Fixed a bycatch JS error related to changing datasets.

  5. Adjusted the default zoom level as @villebro suggested

@@ -124,8 +124,8 @@ export interface BaseFormData extends TimeRange, FormDataResidual {
groupby?: QueryFormColumn[];
all_columns?: QueryFormColumn[];
/** list of filters */
adhoc_filters?: AdhocFilter[];
extra_filters?: QueryFormExtraFilter[];
adhoc_filters?: AdhocFilter[] | null;
Copy link
Contributor

Choose a reason for hiding this comment

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

adhoc_filters may be null when switching datasets, causing this JS error if you hit Cmd + Enter after switching to a new dataset:

js-error

@ktmud
Copy link
Contributor

ktmud commented Feb 19, 2021

Looking good! @mayurnewase Thanks for all the iterations!

Merging to keep your sanity! 😄

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

Successfully merging this pull request may close these issues.

None yet

5 participants