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(animation): support multi-level drill-down for universal transition #17611

Merged
merged 25 commits into from
Oct 24, 2022

Conversation

tyn1998
Copy link
Contributor

@tyn1998 tyn1998 commented Sep 2, 2022

Brief Information

Multiple level drill down support for the universalTransition feature.

This pull request is in the type of:

  • bug fixing
  • new feature
  • others

What does this PR do?

In this PR, a new property childGroupId comes up and together with groupId, they can build multiple "parent-child" relationships between options. The old way of only using groupId to implement a one-level drilldown and other animations still works. I have reviewed all test cases related to universalTransition and everything remains fine because the new API does not bring any breaking change.

It is necessary to decide whether groupId or childGroupId is used as key in the keyGetter funtion to make sure that universalTransition animation does right, so I figured out a way to handle this, please see the block comments in code.

A test html file is added to illustrate how to use childGroupId with groupId to implement the effect of multiple level down drill.

Also fixed #17309 by the way, so #17559 can be closed if this PR is merged (I just cherry picked the commit in that PR to this branch).

Fixed issues

Details

Before: What was the problem?

Only 1 level drill down is allowed.

After: How does it behave after the fixing?

Multiple level drill down is allowed:

2022-09-22.21.53.47.mov

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

N.A.

Others

Merging options

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

Other information

This small feature is an OPSS2022 task, thank you guys from echarts core team to give me so much help when I was developing it!

@echarts-bot echarts-bot bot added PR: awaiting doc Document changes is required for this PR. PR: awaiting review labels Sep 2, 2022
@echarts-bot
Copy link

echarts-bot bot commented Sep 2, 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.

@tyn1998
Copy link
Contributor Author

tyn1998 commented Sep 2, 2022

I will add more test cases to testify if everything works fine:

  • N old series --> N new series
  • drilldown/aggregate between different types of serieses
  • Get groupId from raw dataItem/dataGroupId

@@ -644,6 +765,7 @@ export function installUniversalTransition(registers: EChartsExtensionInstallReg

// TODO multiple to multiple series.
if (globalStore.oldSeries && params.updatedSeries && params.optionChanged) {
// TODO transitionOpt was used in an old implementation and can be removed now
Copy link
Contributor Author

@tyn1998 tyn1998 Sep 2, 2022

Choose a reason for hiding this comment

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

Hi, @pissang.

As you mentioned earlier that transitionSeriesFromOpt is an old implementation but not removed from code, should I remove those code related?

Comment on lines -212 to -219
// If specified key dimension(itemGroupId by default). Use this same dimension from other data.
// PENDING: If only use key dimension of newData.
const keyDim = isOld
? (oldKeyDim || newKeyDim)
: (newKeyDim || oldKeyDim);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This fault-tolerance code is simply dropped. Because the code for getting key from dimension(encode) is brought forward, where accessing both oldKeyDim and newKeyDim is not convinient.

In my opinion, the code should better be dropped. Users should get universalTransition working only if they satisfy all needs to trigger a universalTransition as the doc says. If they do not provide a perfect option but our code helps handling what they miss to make universalTransition succeed, they might be confused once they find "a bad option also triggers universalTransition", since they have no idea that echarts developers write fault-tolerance code to help with their bad options.

@Ovilia Ovilia added this to the 5.4.1 milestone Sep 22, 2022
@echarts-bot echarts-bot bot added PR: doc ready and removed PR: awaiting doc Document changes is required for this PR. labels Sep 22, 2022
src/animation/universalTransition.ts Outdated Show resolved Hide resolved
src/animation/universalTransition.ts Outdated Show resolved Hide resolved
src/animation/universalTransition.ts Outdated Show resolved Hide resolved
src/animation/universalTransition.ts Outdated Show resolved Hide resolved
src/animation/universalTransition.ts Outdated Show resolved Hide resolved
src/util/types.ts Outdated Show resolved Hide resolved
@pissang pissang modified the milestones: 5.4.1, 5.5.0 Oct 16, 2022
@pissang pissang changed the base branch from master to next October 16, 2022 07:59
@tyn1998
Copy link
Contributor Author

tyn1998 commented Oct 17, 2022

Add another test case that shows the number of git commits in two github organizations Org X and Org Y:

2022-10-17.15.08.34.mov

In this test case, I put all data into raw dataItems directly, not by a dataset. And it also works fine.

@pissang
Copy link
Contributor

pissang commented Oct 18, 2022

@tyn1998 Looks great!

@pissang pissang changed the base branch from next to master October 22, 2022 13:29
@pissang pissang changed the base branch from master to next October 22, 2022 13:29
src/animation/universalTransition.ts Outdated Show resolved Hide resolved
src/animation/universalTransition.ts Outdated Show resolved Hide resolved
src/animation/universalTransition.ts Outdated Show resolved Hide resolved
src/animation/universalTransition.ts Outdated Show resolved Hide resolved
@quillblue
Copy link
Contributor

Looks great to me. Thanks @tyn1998 for your excellent contribution!

@pissang pissang merged commit 1dadcab into apache:next Oct 24, 2022
@echarts-bot
Copy link

echarts-bot bot commented Oct 24, 2022

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

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.

[Bug] incorrect dataGroupId for old data items in universalTransition
4 participants