Skip to content

fix: align tooltip z-index to EUI tooltip z-index#931

Merged
markov00 merged 9 commits intoelastic:masterfrom
markov00:2020_12_02-fix_tooltip_z-index
Jan 6, 2021
Merged

fix: align tooltip z-index to EUI tooltip z-index#931
markov00 merged 9 commits intoelastic:masterfrom
markov00:2020_12_02-fix_tooltip_z-index

Conversation

@markov00
Copy link
Copy Markdown
Collaborator

@markov00 markov00 commented Dec 2, 2020 •
edited
Loading

Summary

This PR introduces and uses the z-index computed on the chart parent when the stack context split begins.
The z-index is applied to the tooltip portal with a +100 value. It should be enough to correctly display tooltip above the current embeddable panel (above the title/header of a panel in Kibana dashboard) and below the current Navigation bar (Kibana)

The storybook style is adapted to use a few z-index values on the various root objects. This should suffice the tooltip rendering tests.

close #922

@markov00 markov00 added enhancement New feature or request :tooltip Related to hover tooltip labels Dec 2, 2020
@markov00 markov00 requested a review from nickofthyme December 2, 2020 15:27
@codecov-io
Copy link
Copy Markdown

codecov-io commented Dec 2, 2020 •
edited
Loading

Codecov Report

Merging #931 (7365b3b) into master (f9218ad) will increase coverage by 0.50%.
The diff coverage is 71.15%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #931      +/-   ##
==========================================
+ Coverage   70.61%   71.11%   +0.50%     
==========================================
  Files         343      360      +17     
  Lines       11012    11363     +351     
  Branches     2386     2416      +30     
==========================================
+ Hits         7776     8081     +305     
- Misses       3222     3262      +40     
- Partials       14       20       +6
Flag Coverage Δ
unittests 70.60% <71.15%> (-0.01%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
src/components/portal/tooltip_portal.tsx 10.95% <10.00%> (-1.35%) ⬇️
src/components/portal/utils.ts 51.51% <70.00%> (+22.94%) ⬆️
...rt/renderer/dom/annotations/annotation_tooltip.tsx 62.06% <100.00%> (ø)
.../xy_chart/renderer/dom/annotations/annotations.tsx 68.42% <100.00%> (+1.14%) ⬆️
...rc/chart_types/xy_chart/renderer/dom/crosshair.tsx 80.35% <100.00%> (+0.35%) ⬆️
.../chart_types/xy_chart/renderer/dom/highlighter.tsx 64.86% <100.00%> (+0.97%) ⬆️
src/components/chart.tsx 77.77% <100.00%> (+1.77%) ⬆️
src/components/tooltip/tooltip.tsx 64.35% <100.00%> (+0.35%) ⬆️
src/state/actions/z_index.ts 100.00% <100.00%> (ø)
src/state/chart_state.ts 87.91% <100.00%> (+0.41%) ⬆️
... and 17 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f9218ad...7365b3b. Read the comment docs.

@markov00
fix: reduce z-index to align with kibana
aa8e56c 
I've moved the z-index value to a decent value for kibana to appear below the current navigation bars
@nickofthyme
Copy link
Copy Markdown
Collaborator

nickofthyme commented Dec 3, 2020

@markov00 is this still a draft? Or is it ready for review?

@markov00 markov00 added the wip work in progress label Dec 9, 2020
@markov00
Copy link
Copy Markdown
Collaborator Author

markov00 commented Dec 9, 2020

@nickofthyme it still a draft: we can assign a hardcoded z-index, but we should probably use a different way to compute the right z-index for tooltip.
We probably need the same strategy applied by EUI popover: https://github.com/elastic/eui/blob/43ccf53054cc7ad364778d90c25ad781886d3a09/src/services/popover/popover_positioning.ts#L737 we should compute the z-index of the chart element in the dom and use that index accordingly.
I just wondering what would be the best way to apply that computed z-index: should I compute it during the componentDidMount of the Chart component and update the state with that value?

@markov00 markov00 removed the wip work in progress label Dec 9, 2020
@markov00
Copy link
Copy Markdown
Collaborator Author

markov00 commented Dec 9, 2020

I think everything is ok now for a review @nickofthyme

nickofthyme
nickofthyme approved these changes Dec 22, 2020
View reviewed changes
Copy link
Copy Markdown
Collaborator

@nickofthyme nickofthyme left a comment

Choose a reason for hiding this comment

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

LTGM, much nicer with the stacking traversing logic. Tested stories for z-index changes and see no regressions 🎉.

@markov00 markov00 merged commit ffd626b into elastic:master Jan 6, 2021
@markov00 markov00 deleted the 2020_12_02-fix_tooltip_z-index branch January 6, 2021 16:13
github-actions bot pushed a commit that referenced this pull request Jan 30, 2021
@semantic-release-bot
chore(release): 24.5.0 [skip ci]
d040f71 
# [24.5.0](v24.4.0...v24.5.0) (2021-01-30)

### Bug Fixes

* add theme min radius to point shape ([#996](#996)) ([eb37175](eb37175))
* align tooltip z-index to EUI tooltip z-index ([#931](#931)) ([ffd626b](ffd626b))
* chart state and series functions cleanup ([#989](#989)) ([944ac6c](944ac6c))
* create unique ids for dot icons ([#971](#971)) ([e1ce768](e1ce768))
* external tooltip legend extra value sync ([#993](#993)) ([13ad05a](13ad05a))
* **legend:** disable focus and keyboard navigation for legend in partition ch… ([#952](#952)) ([03bd2f7](03bd2f7))
* **legend:** hierarchical legend order should follow the tree paths ([#947](#947)) ([f9218ad](f9218ad)), closes [#944](#944)
* **legend:** remove ids for circles ([#973](#973)) ([b3f4f90](b3f4f90))

### Features

* **cursor:** improve theme styling for crosshair ([#980](#980)) ([6c4dafd](6c4dafd))
* **legend:**  display pie chart legend extra ([#939](#939)) ([d14de01](d14de01))
* **legend:** add keyboard navigation ([#880](#880)) ([87c227d](87c227d))
* **partition:** Flame and icicle chart ([#965](#965)) ([3df73d0](3df73d0))
* **partition:** legend hover options ([#978](#978)) ([f810d94](f810d94))
* **xy:** support multiple point shapes on line, area and bubble charts ([#988](#988)) ([1392b7d](1392b7d))
@markov00
Copy link
Copy Markdown
Collaborator Author

markov00 commented Jan 30, 2021

🎉 This PR is included in version 24.5.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@markov00 markov00 added the released Issue released publicly label Jan 30, 2021
AMoo-Miki pushed a commit to AMoo-Miki/OpenSearch-Dashboards that referenced this pull request Feb 10, 2022
@semantic-release-bot
chore(release): 24.5.0 [skip ci]
b3d1a20 
# [24.5.0](elastic/elastic-charts@v24.4.0...v24.5.0) (2021-01-30)

### Bug Fixes

* add theme min radius to point shape ([opensearch-project#996](elastic/elastic-charts#996)) ([98089a9](elastic/elastic-charts@98089a9))
* align tooltip z-index to EUI tooltip z-index ([opensearch-project#931](elastic/elastic-charts#931)) ([f7f1f6f](elastic/elastic-charts@f7f1f6f))
* chart state and series functions cleanup ([opensearch-project#989](elastic/elastic-charts#989)) ([42a7af0](elastic/elastic-charts@42a7af0))
* create unique ids for dot icons ([opensearch-project#971](elastic/elastic-charts#971)) ([0b3e00f](elastic/elastic-charts@0b3e00f))
* external tooltip legend extra value sync ([opensearch-project#993](elastic/elastic-charts#993)) ([7e1096e](elastic/elastic-charts@7e1096e))
* **legend:** disable focus and keyboard navigation for legend in partition ch… ([opensearch-project#952](elastic/elastic-charts#952)) ([dfff3e2](elastic/elastic-charts@dfff3e2))
* **legend:** hierarchical legend order should follow the tree paths ([opensearch-project#947](elastic/elastic-charts#947)) ([7b70186](elastic/elastic-charts@7b70186)), closes [opensearch-project#944](elastic/elastic-charts#944)
* **legend:** remove ids for circles ([opensearch-project#973](elastic/elastic-charts#973)) ([ed98481](elastic/elastic-charts@ed98481))

### Features

* **cursor:** improve theme styling for crosshair ([opensearch-project#980](elastic/elastic-charts#980)) ([0248ad6](elastic/elastic-charts@0248ad6))
* **legend:**  display pie chart legend extra ([opensearch-project#939](elastic/elastic-charts#939)) ([672a4df](elastic/elastic-charts@672a4df))
* **legend:** add keyboard navigation ([opensearch-project#880](elastic/elastic-charts#880)) ([b471a94](elastic/elastic-charts@b471a94))
* **partition:** Flame and icicle chart ([opensearch-project#965](elastic/elastic-charts#965)) ([9e8b1f7](elastic/elastic-charts@9e8b1f7))
* **partition:** legend hover options ([opensearch-project#978](elastic/elastic-charts#978)) ([acd1339](elastic/elastic-charts@acd1339))
* **xy:** support multiple point shapes on line, area and bubble charts ([opensearch-project#988](elastic/elastic-charts#988)) ([4f23b4f](elastic/elastic-charts@4f23b4f))
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@nickofthyme nickofthyme nickofthyme approved these changes

Assignees

No one assigned

Labels

enhancement New feature or request released Issue released publicly :tooltip Related to hover tooltip

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Expose tooltip z-index

3 participants

@markov00 @codecov-io @nickofthyme