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

fix: annotation tooltip display when remounting specs #1167

Merged
merged 3 commits into from
May 27, 2021

Conversation

nickofthyme
Copy link
Collaborator

@nickofthyme nickofthyme commented May 26, 2021

Summary

fix #1063

Fixes issue where annotation tooltip would not render when hovering annotation marker when using useContext.

Details

The main issue here is that apm uses useContext on the onPointerUpdate listener callback. This causes the chart to rerender and re-upsert the chart specs. Doing this changes the object reference between hoveredDOMElement.datum and the datum in annotationDimensions map.

This was a check to ensure the correct tooltip is rendered but since the is of all specs must be unique, this check can be removed without concern.

This is a demo of the linked charts library with the changes included in this PR. Notice the annotation tooltip being shown in all hover cases.

Screen.Recording.2021-05-26.at.10.38.22.AM.mp4

Checklist

  • Unit tests were updated or added to match the most common scenarios

@nickofthyme nickofthyme added bug Something isn't working :annotation Annotation (line, rect, text) related issue labels May 26, 2021
@nickofthyme nickofthyme requested review from markov00 and rshen91 May 26, 2021 15:48
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.

Perfectly agree, thanks Nick for the investigation on this.
The id is used here comes from getAnnotationLinePropsId that already take in consideration the following properties:

[specId, verticalValue, horizontalValue, datum.header, datum.details, index]

and can be considered unique

@nickofthyme nickofthyme reopened this May 27, 2021
@nickofthyme nickofthyme enabled auto-merge (squash) May 27, 2021 16:44
@nickofthyme nickofthyme merged commit 8408600 into elastic:master May 27, 2021
@nickofthyme nickofthyme deleted the fix-annotation-tooltip branch May 28, 2021 02:06
nickofthyme pushed a commit that referenced this pull request Jun 4, 2021
# [30.0.0](v29.2.0...v30.0.0) (2021-06-04)

### Bug Fixes

* **domain:** custom domain should not filter data ([#1181](#1181)) ([76e8dca](76e8dca)), closes [#1129](#1129)
* **value_labels:** zero as a valid value for textBorder and borderWidth ([#1182](#1182)) ([a64f333](a64f333))
* annotation tooltip display when remounting specs ([#1167](#1167)) ([8408600](8408600))
* render nodeLabel formatted text into the nodes ([#1173](#1173)) ([b44bdff](b44bdff))

### Features

* **axis:** allow pixel domain padding for y axes  ([#1145](#1145)) ([7c1fa8e](7c1fa8e))
* apply value formatter to the default legend item label ([#1190](#1190)) ([71474a5](71474a5))
* **tooltip:** stickTo vertical middle of the cursor ([#1163](#1163)) ([380363b](380363b)), closes [#1108](#1108)
* **wordcloud:** click and over events on text ([#1180](#1180)) ([196fb6a](196fb6a)), closes [#1156](#1156)

### BREAKING CHANGES

* **value_labels:** the `textBorder` of `ValueFillDefinition` is now optional or a number only
@nickofthyme
Copy link
Collaborator Author

🎉 This PR is included in version 30.0.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@nickofthyme nickofthyme added the released Issue released publicly label Jun 4, 2021
AMoo-Miki pushed a commit to AMoo-Miki/OpenSearch-Dashboards that referenced this pull request Feb 10, 2022
# [30.0.0](elastic/elastic-charts@v29.2.0...v30.0.0) (2021-06-04)

### Bug Fixes

* **domain:** custom domain should not filter data ([opensearch-project#1181](elastic/elastic-charts#1181)) ([92ba84c](elastic/elastic-charts@92ba84c)), closes [opensearch-project#1129](elastic/elastic-charts#1129)
* **value_labels:** zero as a valid value for textBorder and borderWidth ([#1182](elastic/elastic-charts#1182)) ([880fbf1](elastic/elastic-charts@880fbf1))
* annotation tooltip display when remounting specs ([opensearch-project#1167](elastic/elastic-charts#1167)) ([7163951](elastic/elastic-charts@7163951))
* render nodeLabel formatted text into the nodes ([opensearch-project#1173](elastic/elastic-charts#1173)) ([0de9688](elastic/elastic-charts@0de9688))

### Features

* **axis:** allow pixel domain padding for y axes  ([#1145](elastic/elastic-charts#1145)) ([6787728](elastic/elastic-charts@6787728))
* apply value formatter to the default legend item label ([opensearch-project#1190](elastic/elastic-charts#1190)) ([20108bb](elastic/elastic-charts@20108bb))
* **tooltip:** stickTo vertical middle of the cursor ([#1163](elastic/elastic-charts#1163)) ([b858fb3](elastic/elastic-charts@b858fb3)), closes [opensearch-project#1108](elastic/elastic-charts#1108)
* **wordcloud:** click and over events on text ([opensearch-project#1180](elastic/elastic-charts#1180)) ([adbf341](elastic/elastic-charts@adbf341)), closes [opensearch-project#1156](elastic/elastic-charts#1156)

### BREAKING CHANGES

* **value_labels:** the `textBorder` of `ValueFillDefinition` is now optional or a number only
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:annotation Annotation (line, rect, text) related issue bug Something isn't working released Issue released publicly
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Annotation tooltip not being displayed.
2 participants