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

fix(tooltip): custom tooltip header context #1989

Merged
merged 12 commits into from
Apr 3, 2023
Merged

fix(tooltip): custom tooltip header context #1989

merged 12 commits into from
Apr 3, 2023

Conversation

nickofthyme
Copy link
Collaborator

@nickofthyme nickofthyme commented Mar 7, 2023 •
edited
Loading

Summary

The header of the TooltipInfo used to pull the tooltip value from the first/top value in the list of tooltip values. This behavior was unorthodox and unreliable. We have changed the TooltipInfo type to better represent the header and values.

BREAKING CHANGES: The header property of TooltipInfo type was simplified to PointerValue as to include only relevant properties. This change is propagated to all other types using header as a TooltipValue. The TooltipInfo.values used to conditionally pass only highlighted TooltipValues when using a customTooltip and now always passes all values.

Issues

fixes #1988

Checklist

  • The proper chart type label has been added (e.g. :xy, :partition)
  • The proper feature labels have been added (e.g. :interactions, :axis)
  • The :theme label has been added and the @elastic/eui-design team has been pinged when there are Theme API changes
  • All related issues have been linked (i.e. closes #123, fixes #123)
  • New public API exports have been added to packages/charts/src/index.ts
  • Unit tests have been added or updated to match the most common scenarios
  • The proper documentation and/or storybook story has been added or updated

@nickofthyme nickofthyme added the :tooltip Related to hover tooltip label Mar 7, 2023
@markov00 markov00 changed the title fix(tooltip): custom tooltip heaader context fix(tooltip): custom tooltip header context Mar 7, 2023
@markov00
Copy link
Member

markov00 commented Mar 7, 2023

So checking the code I think we should:

  • in cartesian charts, thetooltip.values should always return the full set of values under the same X value (this doesn't happen if we apply the fix in this way)
  • the header usually doesn't represent exactly the top, highlighted element, but just the first on the list. We usually don't care about that because what we use on the header of the tooltip is generally the X value that is available in the formattedValue (const value = isHeader ? x : y; in packages/charts/src/chart_types/xy_chart/tooltip/tooltip.ts)
    The label value instead is "wrong" actually and we can probably replace it with an empty string.
    Also the datum is wrong, because we should just extract the current x value. So probably, in the customTooltip situation, we could return just a null header and we could give the user che choice to extract the information they need from the values.
    Alternatively we can return a TooltipValue with no datum and empty label, or a label that reflect the x axis title.

@nickofthyme
chore: always return full values array
e3f8d1e
@nickofthyme
refactor: tooltip header values
04f2f79 
- change header from TooltipValue to PointerValue type
- update tooltip header formatter type
- return full set of values for customTooltips
@nickofthyme
Merge branch 'main' into fix-tooltip-context
cc78c12
@nickofthyme nickofthyme marked this pull request as ready for review March 21, 2023 18:39
@nickofthyme nickofthyme added the API Changes the external API types label Mar 21, 2023
@nickofthyme nickofthyme enabled auto-merge (squash) March 21, 2023 21:33
markov00
markov00 approved these changes Mar 31, 2023
View reviewed changes
Copy link
Member

@markov00 markov00 left a comment

Choose a reason for hiding this comment

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

Changes looks good to me, remember to mark this as breaking change

packages/charts/src/components/tooltip/components/tooltip_header.tsx Outdated Show resolved Hide resolved
@nickofthyme nickofthyme enabled auto-merge (squash) March 31, 2023 23:38
@nickofthyme nickofthyme merged commit 1e5b861 into elastic:main Apr 3, 2023
@nickofthyme nickofthyme deleted the fix-tooltip-context branch April 3, 2023 15:05
nickofthyme pushed a commit that referenced this pull request Apr 18, 2023
@semantic-release-bot
chore(release): 56.0.0 [skip ci]
b613189 
# [56.0.0](v55.0.0...v56.0.0) (2023-04-18)

### Bug Fixes

* **deps:** update dependency @elastic/eui to ^76.4.0 ([#2008](#2008)) ([95176e1](95176e1))
* **deps:** update dependency @elastic/eui to v77 ([#2018](#2018)) ([c079730](c079730))
* **interactions:** brushing over origin coordinates ([#2013](#2013)) ([937feb0](937feb0))
* **tooltip:** custom tooltip header context ([#1989](#1989)) ([1e5b861](1e5b861))

### Features

* **metric:** trend with string value ([#2011](#2011)) ([91d7695](91d7695))

### BREAKING CHANGES

* **tooltip:** The `header` property of `TooltipInfo` type was simplified to `PointerValue` as to include only relevant properties. This change is propagated to all other types using `header` as a `TooltipValue`. The `TooltipInfo.values` used to conditionally pass only highlighted `TooltipValue`s when using a `customTooltip` and now _always_ passes all `values`.
@nickofthyme nickofthyme mentioned this pull request May 24, 2023
Merged
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@markov00 markov00 markov00 approved these changes

Assignees
No one assigned
Labels
API Changes the external API types breaking change :tooltip Related to hover tooltip
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Tooltip] customTooltip provides incorrect header value
2 participants
@nickofthyme @markov00