Skip to content

feat(tooltip): inject tooltip position in formatter and data params's point coordinates #18715

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

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

Conversation

MrSquaare
Copy link

Brief Information

This pull request is in the type of:

  • bug fixing
  • new feature
  • others

What does this PR do?

Adds tooltip position as tooltip.formatter argument and injects graphic element's point coordinates into CallbackDataParams.

Fixed issues

Details

Before: What was the problem?

It was not possible to format the tooltip based on its position relative to the points.

After: How does it behave after the fixing?

The user can use the tooltip's position argument and the point coordinates in the params argument of tooltip.formatter to customize the tooltip display.
For example: Highlight nearest serie in tooltip, displays only the nearest point in the tooltip.

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

  • tooltip-formatter test case

Others

Merging options

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

Other information

IMPORTANT NOTE: I retrieved the coordinates from the graphic element of the data, but I'm not sure this is the right way to proceed. I'd be pleased to receive feedback on this.

@echarts-bot
Copy link

echarts-bot bot commented Jun 5, 2023

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.

Thanks for your contribution. I think providing which series is in focus can be helpful in some cases. However, returing position in the callback is not intuitive. As you can see in your demo in the test, the user has to write a lot of code to accomplish the job. So I would suggest this information should be returned in the params series like:

formatter: params => {
    params[0].state // 'normal' | 'emphasis' | 'select' | 'blur'
}

@plainheart plainheart added the discussion-required Further discussion is required before we decide if this issue should be fixed. label Jun 6, 2023
@MrSquaare
Copy link
Author

@Ovilia In fact, it would be more user-friendly to provide state to the user (can you define each of the state you've given me as examples?).
I still think we should return the positions to allows users to handle more complex cases (e.g. I want to display the 3 points closest to my cursor, I want to display the points within a radius, etc.)

@Ovilia
Copy link
Contributor

Ovilia commented Jun 13, 2023

If you want to provide the position, I think it may be a better idea to be the position of the mouse position relative to the canvas top-left corder. Your current code seems to return the x and y of the interacted element, which may not be necessarily be the top-left of the interacted element because it's used for transforming.

Element states are defined in https://github.com/apache/echarts/blob/master/src/util/types.ts#L650

Please also simplify your test case data and remove the unnecessary data.

@MrSquaare
Copy link
Author

@Ovilia The mouse position is already provided as a fourth parameter of tooltip.formatter (sadly this function already takes 3 non-named parameter, maybe should I return the position as a named parameter for future API changes?)
param.x and param.y are the position of the element (I suppose the top left position of the element)
From my tests, I deduced that these position were the element position relative to the top left canvas position (e.g.: setting absolute position to the canvas, seeing that x and y are not related to the canvas's DOM position). I can't be 100% sure that this is always the case (perhaps there are other transformations applied with certain options); No definition is provided for these two ZRender.Element properties (in the code or in the ZRender documentation).

For the element states, I'm not sure if this is something I have to calculate (how?) or if this is something I can already get; currently, from what I see after a code search, DisplayState is mainly used for labels, and the state is provided via a callback to the label options.
I also see that ECElement has two states that correspond to DisplayState: ECElement.hoverState and ECElement.selected;
ECElement also has an applyElementStates method for adding states to the ZRender element. Isn't this done after TooltipView calls getDataParams? This would mean that it would be impossible for me, without changing the order of proceedings, to retrieve these states.
Note that all this may be a confusion on my part and be two separate things.

To conclude, I'm new to the codebase, I'd like to do my best, but I don't know where to find the resources to help me.
In fact, if retrieving the states requires kind of major changes to the codebase, I won't be able to make them. However, I'd be interested in following the development.

Does this kind of discussion have to take place as comments on this PR, or does echarts have specific message channels that I can use?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discussion-required Further discussion is required before we decide if this issue should be fixed. PR: awaiting doc Document changes is required for this PR. PR: first-time contributor PR: revision needed size/S
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants