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(visualMap): able to set the 'dimension' to an array #20703

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

Justin-ZS
Copy link

Brief Information

This pull request is in the type of:

  • bug fixing
  • new feature
  • others

What does this PR do?

able to set the 'dimension' to an array.
item in array will be mapped to target series by index.
official demo

{
  // no series index
  dimension: [1,2,3] // seriesIndex: 0 -> 1, 1 -> 2, 2 -> 3
}
{
  // no series index
  dimension: [1,2] // seriesIndex: 0 -> 1, 1 -> 2, 2 -> 2 missmatched series will use the last dimension
}
{
  seriesIndex: [1, 2]
  dimension: [2,3] // seriesIndex: 1 -> 2, 2 -> 3
}

Fixed issues

#20662

Details

Before: What was the problem?

#20662

After: How does it behave after the fixing?

able to set the 'dimension' to an array.
item in array will be mapped to target series by index.

Document Info

One of the following should be checked.

  • This PR doesn't relate to document changes
  • The document should be updated later
  • The document changes have been made in apache/echarts-doc#xxx

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

@echarts-bot echarts-bot bot added PR: awaiting doc Document changes is required for this PR. PR: awaiting review labels Jan 20, 2025
Copy link

echarts-bot bot commented Jan 20, 2025

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.

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 don't think it's intuitive to use dimension: [1,2] to represent seriesIndex: 0 -> 1, 1 -> 2, 2 -> 2. Instead, it's more like using both dimension 1 and 2.

I think the problem of #20662 should be best solved using the third demo @MatthiasMert gave. I would suggest closing this PR unless a more solid reason is given. Thanks for your contribution anyway.

@Justin-ZS
Copy link
Author

I don't think it's intuitive to use dimension: [1,2] to represent seriesIndex: 0 -> 1, 1 -> 2, 2 -> 2. Instead, it's more like using both dimension 1 and 2.

I think the problem of #20662 should be best solved using the third demo @MatthiasMert gave. I would suggest closing this PR unless a more solid reason is given. Thanks for your contribution anyway.

Using the same visualMap on 2d dataset requires the dataset to be splitted looks overkill.
dimension: [1,2] cannot be explained as using both dimension 1 and 2 since one series can use only one dimension
using both dimension 1 and 2 is technically impossible, unless you choose to provide a function to aggreagte multiple values

@Justin-ZS Justin-ZS requested a review from Ovilia January 21, 2025 09:34
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.

It's true that using both dimension 1 and 2 is technically impossible. Even though, it's misleading to provide such API design. Again, I would suggest using the 3rd solution or avoid using dataset in your case. Adding a new feature should take a lot of things into consideration so I would say it's not quite a good design of API unless a better plan is proposed.

@Justin-ZS
Copy link
Author

Justin-ZS commented Jan 22, 2025

It's true that using both dimension 1 and 2 is technically impossible. Even though, it's misleading to provide such API design. Again, I would suggest using the 3rd solution or avoid using dataset in your case. Adding a new feature should take a lot of things into consideration so I would say it's not quite a good design of API unless a better plan is proposed.

Again, the suggested workaround (splitting the dataset) is unacceptable in my use case.

I understand your concerns about the API design.
I do think this PR is a simple and clear solution for similar cases.
We can provide detailed document to clarify the correct uasge.

Even if user is misled into thinking it as using both dimension 1 and 2, user will still use the numeric dimension unless they want to map multiple dimensions.
Then, they will find the correct usage through document or just a few attempts.

if you insist, I will keep this PR open until a better solutiion is proposed.

@Ovilia
Copy link
Contributor

Ovilia commented Jan 22, 2025

Why is the suggested workaround (splitting the dataset) unacceptable?

@Justin-ZS
Copy link
Author

Why is the suggested workaround (splitting the dataset) unacceptable?

It requires huge refactoring (tooltip, axis. series and so on) and maintaining two implementations.
And the splitted datasets may cause other limitations or bugs.
As a workaround for now, I added the same visualmap for each series and only show the first controller.
When selection changes, use setOption to sync the state cross all visualMaps.
But the hoverLink interaction cannot be synced.

@Ovilia
Copy link
Contributor

Ovilia commented Jan 22, 2025

I don't think it should require huge work to refactor from dataset to not. And besides, you can try with AI to help.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: awaiting doc Document changes is required for this PR. PR: revision needed size/M
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants