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(sankey): support trajectory for emphasis state #17451

Merged
merged 7 commits into from Mar 9, 2023

Conversation

ElayGelbart
Copy link
Contributor

@ElayGelbart ElayGelbart commented Aug 1, 2022

Graph Emphasis Full Path:

This pull request is in the type of:

  • bug fixing
  • new feature
  • others

this pull request adding the functionality to emphasis the full path from start to finish on hover/highlight event

Fixed issues

Details

Before: What was the problem?

There was only three options, 'self','series','adjacency'. it was needed to emphasis the whole path of nodes/edges to highlight all the related paths to specific node/edge

'adjacency' provide only one before one after emphasis.

image

After: How does it behave after the fixing?

after we can have trajectory option on emphasis:focus

chaer.setOption({
  series: [
    {
      type: 'sankey',
      nodes: data.nodes,
      links: data.links,
      lineStyle: {
        color: 'gradient',
        curveness: 0.5
      },
      emphasis: {
        focus: 'trajectory'
      }
    }
  ];
});

image

Document Info

One of the following should be checked.

Misc

ZRender Changes

  • This PR depends on ZRender changes (ecomfe/zrender#xxx).

Related test cases or examples to use the new APIs

Please refer to test/emphasis-fullPath.html

Others

Merging options

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

Other information

@echarts-bot
Copy link

echarts-bot bot commented Aug 1, 2022

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.

Document changes are required in this PR. Please also make a PR to apache/echarts-doc for document changes and update the issue id in the PR description. When the doc PR is merged, the maintainers will remove the PR: awaiting doc label.

@ElayGelbart
Copy link
Contributor Author

@echarts-bot echarts-bot bot added PR: doc ready and removed PR: awaiting doc Document changes is required for this PR. labels Aug 1, 2022
@ElayGelbart
Copy link
Contributor Author

ElayGelbart commented Aug 23, 2022

Hey I know you're busy, when this will get review ? @plainheart @pissang

Copy link
Contributor

@Ovilia Ovilia left a comment

Choose a reason for hiding this comment

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

I think this feature is cool! Thanks for your contribution. I'm just checking a few basic problems here.

src/chart/graph/GraphSeries.ts Outdated Show resolved Hide resolved
src/chart/sankey/SankeyView.ts Show resolved Hide resolved
src/data/Graph.ts Outdated Show resolved Hide resolved
@Ovilia Ovilia modified the milestones: 5.4, 5.4.1 Sep 1, 2022
@plainheart plainheart modified the milestones: 5.4.1, 5.5.0 Nov 10, 2022
@ElayGelbart
Copy link
Contributor Author

Is there any release date for this PR merge ?

Copy link
Contributor

@Ovilia Ovilia left a comment

Choose a reason for hiding this comment

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

Sorry for late reply. Our next version is to be released in Feb 2023.

src/data/Graph.ts Outdated Show resolved Hide resolved
src/data/Graph.ts Outdated Show resolved Hide resolved
test/emphasis-fullPath.html Outdated Show resolved Hide resolved
@ElayGelbart ElayGelbart requested review from plainheart and Ovilia and removed request for Ovilia and plainheart February 22, 2023 16:28
@ElayGelbart
Copy link
Contributor Author

@plainheart Fixed code by your recommendations

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.

LGTM. Let's see if @Ovilia has other comments.

Copy link
Contributor

@Ovilia Ovilia left a comment

Choose a reason for hiding this comment

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

I think this is a bug, right?
image

@ElayGelbart
Copy link
Contributor Author

I think this is a bug, right? image

Hey @Ovilia , This is not a bug, Liquid node is connected to Bio-conversion node...

@ElayGelbart ElayGelbart requested a review from Ovilia March 5, 2023 07:51
@plainheart
Copy link
Member

@Ovilia I agree with @ElayGelbart. It's a bit hard to see but they do connect with a very thin line.

Copy link
Contributor

@Ovilia Ovilia left a comment

Choose a reason for hiding this comment

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

I see! Thanks for your contribution.

@Ovilia Ovilia merged commit 0be0cfc into apache:master Mar 9, 2023
@echarts-bot
Copy link

echarts-bot bot commented Mar 9, 2023

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

@plainheart plainheart modified the milestones: 5.5.0, 5.4.3 Jun 25, 2023
@lhd163
Copy link

lhd163 commented Sep 6, 2023

发现个问题:focus 配置为 'trajectory' 时,当节点数 data,超过 100 ,边 links 超过 300 的时候,会导致页面崩溃

@plainheart
Copy link
Member

@lhd163 Thanks for your feedback. Please file a bug report with a reproduction example.

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.

[Feature] emphasis full path of link/node from start to finish of chart
6 participants