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

feat(graph): graph support multiple edges, for #6811 #12590

Merged
merged 10 commits into from
Jun 29, 2020

Conversation

wf123537200
Copy link
Contributor

the type=graph support multiple edges between two nodes

This pull request is in the type of:

  • bug fixing
  • new feature
  • others

What does this PR do?

the type=graph support multiple edges between two nodes

Fixed issues

#6811

Details

Before: What was the problem?

image

After: How is it fixed in this PR?

image

Usage

Are there any API changes?

  • The API has been changed.

series > autoCurveness

// if number will set curveness list max length with params
// if function will use the return array as the curveness list 
autoCurveness: 'number | function() {}'

Related test cases or examples to use the new APIs

graph-mulitple-edges.html

Others

Merging options

  • Please squash the commits into a single one when merge.

Other information

@echarts-bot
Copy link

echarts-bot bot commented May 8, 2020

Thanks for your contribution!
The community will review it ASAP. In the meanwhile, please checkout the coding standard and Wiki about How to make a pull request.

@wf123537200
Copy link
Contributor Author

Chinese comments removed~~

@ohadleshno
Copy link

Great feature 👍

ohadleshno
ohadleshno previously approved these changes May 16, 2020
@stolenfather
Copy link

Hey , I really need this pr when will it be merged?

@travis1111
Copy link

Someone please review this :-) we also need this

@pissang pissang added this to the 4.9.0 milestone May 26, 2020
@bionexit
Copy link

Hey , I really need this pr when will it be merged?

This is also my urgent need pr...

src/chart/helper/multipleGraphEdgeHelper.js Outdated Show resolved Hide resolved
src/chart/helper/multipleGraphEdgeHelper.js Outdated Show resolved Hide resolved
test/graph-mulitple-edges.html Outdated Show resolved Hide resolved
src/chart/helper/multipleGraphEdgeHelper.js Show resolved Hide resolved
src/chart/helper/multipleGraphEdgeHelper.js Outdated Show resolved Hide resolved
src/chart/helper/multipleGraphEdgeHelper.js Outdated Show resolved Hide resolved
src/chart/helper/multipleGraphEdgeHelper.js Outdated Show resolved Hide resolved
src/chart/helper/multipleGraphEdgeHelper.js Outdated Show resolved Hide resolved
src/chart/helper/multipleGraphEdgeHelper.js Outdated Show resolved Hide resolved
src/chart/helper/createGraphFromNodeEdge.js Outdated Show resolved Hide resolved
@wf123537200
Copy link
Contributor Author

@100pah Thanks for your suggestions and code review, this is a great help!
I rebuild the code as suggested.
Please help me see if it is appropriate to review the current implementation.

Thanks!

@wf123537200 wf123537200 requested a review from 100pah June 11, 2020 11:51
@100pah
Copy link
Member

100pah commented Jun 28, 2020

@wf123537200
Sorry my review is too late.
There are several places could be enhanced.
The others look good to me.

@wf123537200
Copy link
Contributor Author

@100pah 3q for the review and the suggest, i fixed it.

@100pah 100pah merged commit baae12e into apache:master Jun 29, 2020
@echarts-bot
Copy link

echarts-bot bot commented Jun 29, 2020

Congratulations! Your PR has been merged. Thanks for your contribution! 👍

@100pah
Copy link
Member

100pah commented Jun 29, 2020

@wf123537200 Thanks for your great work!

@plainheart plainheart added the PR: awaiting doc Document changes is required for this PR. label Jun 29, 2020
@plainheart
Copy link
Member

👍 Documentation about this new feature is waiting to be added.

Copy link
Member

@plainheart plainheart left a comment

Choose a reason for hiding this comment

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

Though this PR has been merged, there may be some parts could be optimized.

src/data/Graph.js Show resolved Hide resolved
src/chart/helper/createGraphFromNodeEdge.js Show resolved Hide resolved
src/chart/graph/forceLayout.js Show resolved Hide resolved
src/chart/graph/GraphSeries.js Show resolved Hide resolved
plainheart added a commit to plainheart/echarts that referenced this pull request Jul 2, 2020
plainheart added a commit to plainheart/echarts that referenced this pull request Jul 2, 2020
@plainheart plainheart mentioned this pull request Jul 2, 2020
3 tasks
@wf123537200
Copy link
Contributor Author

@plainheart 3q for fix this

@wf123537200
Copy link
Contributor Author

👍 Documentation about this new feature is waiting to be added.

i will add to documentation ASAP.

plainheart added a commit that referenced this pull request Jul 2, 2020
Ovilia pushed a commit to apache/echarts-doc that referenced this pull request Jul 9, 2020
@plainheart plainheart removed the PR: awaiting doc Document changes is required for this PR. label Jul 9, 2020
@ryancui92
Copy link

ryancui92 commented Sep 2, 2020

I found that in some cases, toggle legend will make multiple lines into single one.(overlapping)

Check it out in this demo: https://gallery.echartsjs.com/editor.html?c=xNQsu5DekH&v=2

Then I figured that edgeArray is undefined after selecting legend, maybe caused by incorrect node.dataIndex.

See here: https://github.com/wf123537200/incubator-echarts/blob/fd25c1a49d72cc32930c9a0836698d1a125b3298/src/chart/helper/multipleGraphEdgeHelper.js#L75

I changed two lines, and it worked.

// var source = [n1.id, n1.dataIndex].join('.');
var source = n1.id;
// var target = [n2.id, n2.dataIndex].join('.');
var target = n2.id;

I am wondering why dataIndex is needed in generating edgeKey here?

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.