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.

Sign up for GitHub

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

Jump to bottom

feat(animation): support multi-level drill-down for universal transition #17611

Merged
merged 25 commits into from
Oct 24, 2022
Merged

feat(animation): support multi-level drill-down for universal transition #17611

merged 25 commits into from
Oct 24, 2022

Conversation

tyn1998
Copy link
Contributor

@tyn1998 tyn1998 commented Sep 2, 2022 •
edited by plainheart
Loading

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 •
edited
Loading

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

tyn1998
tyn1998 commented Sep 2, 2022
View reviewed changes
src/animation/universalTransition.ts
@@ -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 •
edited
Loading

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?

tyn1998
tyn1998 commented Sep 3, 2022
View reviewed changes
src/animation/universalTransition.ts
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
@tyn1998 tyn1998 mentioned this pull request Sep 28, 2022
Merged
2 tasks
pissang
pissang requested changes Oct 15, 2022
View reviewed changes
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 •
edited
Loading

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
pissang
pissang reviewed Oct 22, 2022
View reviewed changes
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! 👍

@plainheart plainheart changed the title feat: multiple level drill down support for universalTransition feat(animation): support multi-level drill-down for universal transition Dec 2, 2022
@tyn1998 tyn1998 mentioned this pull request Mar 14, 2023
Open
@plainheart plainheart mentioned this pull request Jun 20, 2023
Merged
3 tasks
Ovilia pushed a commit that referenced this pull request Jan 18, 2024
@plainheart
feat(animation): support multi-level drill-down for universal transit…
2438562 
…ion #17611
@Ovilia Ovilia mentioned this pull request Jan 25, 2024
Merged
8 tasks
@X-oss-byte X-oss-byte mentioned this pull request May 25, 2024
Open
@chandraguptsharma1 chandraguptsharma1 mentioned this pull request Jul 17, 2024
Open
@junior junior mentioned this pull request Jul 26, 2024
Closed
@X-oss-byte X-oss-byte mentioned this pull request Aug 7, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pissang pissang pissang approved these changes

@quillblue quillblue quillblue approved these changes

Assignees
No one assigned
Labels
PR: doc ready size/L
Projects
None yet
Milestone
5.5.0
Development

Successfully merging this pull request may close these issues.

[Bug] incorrect dataGroupId for old data items in universalTransition
4 participants
@tyn1998 @pissang @quillblue @Ovilia