Skip to content

fix(tooltip): rendering in react v18#2169

Merged
nickofthyme merged 6 commits intoelastic:mainfrom
nickofthyme:fix-tooltip-react-18
Sep 19, 2023
Merged

fix(tooltip): rendering in react v18#2169
nickofthyme merged 6 commits intoelastic:mainfrom
nickofthyme:fix-tooltip-react-18

Conversation

@nickofthyme
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Collaborator Author

buildkite update screenshots

@nickofthyme
Copy link
Copy Markdown
Collaborator Author

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

Copy link
Copy Markdown
Collaborator

@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
@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