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(tooltip): rendering in react v18 #2169

Merged
merged 6 commits into from
Sep 19, 2023

Conversation

nickofthyme
Copy link
Collaborator

@nickofthyme nickofthyme commented Sep 13, 2023

Summary

Fixes issue where tooltip would not render when using react@^18.

Details

Troubleshooting steps:

  • So digging into this at first I noticed that popper was not applying the styles to the portalNode element.
  • I then realized that the styles were mostly zeros which must be why they didn't apply the styles to the element.
  • Next I noticed the popper's computed size of the portalNode element was all zeros including the height and width.
  • Considering the zero height I looked into the popper code and compared between the current main playground and the playground using react@^v18. I noticed that when the size computation was performed, the current code rendered the tooltip size accurately but the react@^v18 did not show a tooltip at all.
  • Capturing the portalNode element ref from the code and searching for it in the dom, nothing! It was missing.
  • Thus the reference to the portalNode was broken.

Why??? I have no clue...

I searched the react changelog for changes to createPortal and saw nothing. But for some reason the old code with react@^16 was able to maintain an active reference to the portal dom node and react@^18 was not.

The solution here is to ensure the portal node reference is maintained across re-renders. Before we did so by creating a new portalNode each time but here I simply append the saved reference node to the dom each render.

Issues

fixes #1910

Checklist

  • The proper chart type label has been added (e.g. :xy, :partition)
  • The proper feature labels have been added (e.g. :interactions, :axis)
  • All related issues have been linked (i.e. closes #123, fixes #123)
  • Unit tests have been added or updated to match the most common scenarios
  • The code has been checked for cross-browser compatibility (Chrome, Firefox, Safari, Edge)

- manually append anchor on render
- limit calls to getOrCreateNode
- upgrade to latest version of @popperjs/core
@nickofthyme nickofthyme added :tooltip Related to hover tooltip dependencies Pull requests that update a dependency file :all Applies to all chart types labels Sep 13, 2023
@nickofthyme nickofthyme changed the title Fix tooltip react 18 fix(tooltip): rendering in react v18 Sep 13, 2023
@nickofthyme
Copy link
Collaborator Author

nickofthyme commented Sep 13, 2023

@markov00 can you take a look at this? The fix is only in 38be8ad. Happy to take ideas. You can test it by pulling down this branch and running

yarn && yarn playground

Make sure you are on node16 or it will complain. Happy to chat tomorrow.

@nickofthyme nickofthyme marked this pull request as ready for review September 14, 2023 14:06
@nickofthyme
Copy link
Collaborator Author

buildkite update screenshots

@nickofthyme
Copy link
Collaborator Author

Slight pixel shift in placement from updated @popperjs/core version

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.

It is fine for me, but we probably should consider a way to test the library with react 18

@nickofthyme nickofthyme merged commit f30df54 into elastic:main Sep 19, 2023
11 checks passed
@nickofthyme nickofthyme deleted the fix-tooltip-react-18 branch September 19, 2023 15:53
nickofthyme pushed a commit that referenced this pull request Sep 20, 2023
# [60.0.0](v59.1.0...v60.0.0) (2023-09-20)

### Bug Fixes

* **deps:** update dependency @elastic/eui to ^88.2.0 ([#2161](#2161)) ([6609a19](6609a19))
* **deps:** update dependency @elastic/eui to ^88.3.0 ([#2163](#2163)) ([624f43a](624f43a))
* **deps:** update dependency @elastic/eui to v85 ([#2113](#2113)) ([1b3fa7c](1b3fa7c))
* **deps:** update dependency @elastic/eui to v87 ([#2145](#2145)) ([312c32c](312c32c))
* **deps:** update dependency @elastic/eui to v88 ([#2154](#2154)) ([4070da0](4070da0))
* **tooltip:** rendering in react v18 ([#2169](#2169)) ([f30df54](f30df54))
* update font family ([#2165](#2165)) ([be07b0c](be07b0c))
* **waffle:** remove alpha artifacts ([#2139](#2139)) ([8eb4ede](8eb4ede))
* Wait a tick before reporting render status ([#2131](#2131)) ([fd2bca4](fd2bca4))
* **xy:** disable legend extra on ordinal ([#2114](#2114)) ([3ddfb18](3ddfb18))

### Features

* add locale prop to Settings ([#2164](#2164)) ([0bb3ab1](0bb3ab1))

### BREAKING CHANGES

* **xy:** when using the `ScaleType.Ordinal` for the X scale the legend extra value, representing the last and current hovered value, will not be shown.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:all Applies to all chart types dependencies Pull requests that update a dependency file :tooltip Related to hover tooltip
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Tooltip doesn't render when React 18 createRoot is used
2 participants