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

Render Tooltip via React Portal #350

Merged
merged 2 commits into from
Aug 1, 2023
Merged

Render Tooltip via React Portal #350

merged 2 commits into from
Aug 1, 2023

Conversation

jahe
Copy link
Contributor

@jahe jahe commented Jul 31, 2023

What:

In this PR we're adding a new prop container to the Tooltip component where the tooltip should be rendered into.

Why:

This PR closes #349.

Tooltips that are rendered in Modal dialogs can cause scrollbars to appear. It was not like that in previous version of wave (1.15.1).

How:

  • Introduce new (optional) prop container to Tooltip component where the tooltip body should be rendered into
  • Render the tooltip body into container via react portal
  • See internal discussion
  • Implementation according to popper library documentation

    Media:

Before:
image

After:
image

Checklist:

  • Ready to be merged

render tooltip via react portal - new prop `container`

closes issue #349
Copy link
Contributor

@nlopin nlopin left a comment

Choose a reason for hiding this comment

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

What do you think if we create a portal by default and attach it to the document.body?
Our users won't even face the issue anymore in that case, the whole issue disappear. I don't see any negative sides on this solution

@jahe
Copy link
Contributor Author

jahe commented Aug 1, 2023

Good idea. We would need to put a big enough z-index on the tooltip then so that it gets rendered on top of modals. With my changes the responsibility for that is delegated to the user of the library which is not very convenient. I agree that it would be better if the library makes sure that it "just works".

create a portal by default

Do you mean using document.body as portal to render tooltips in any case instead of doing it conditionally? In that case we could remove the container prop.

@nlopin
Copy link
Contributor

nlopin commented Aug 1, 2023

Do you mean using document.body as portal to render tooltips in any case instead of doing it conditionally? In that case we could remove the container prop.

Yes, I'd do that. In that case the z-index won't gonna be necessary because the tooltip will be sitting on top of the tree and in a separate stacking context

@jahe
Copy link
Contributor Author

jahe commented Aug 1, 2023

The issue is that Modals come with z-index: 1100 which makes them appear on top of elements that are rendered in a separate stacking context (see example).

remove new `container` prop and render tooltip body in any case via react portal directly to document.body with a z-index of 1300
@jahe
Copy link
Contributor Author

jahe commented Aug 1, 2023

I've removed the container prop in 94b053c. Tooltips are rendered via React Portal into document.body now.

@jahe jahe merged commit 567af48 into main Aug 1, 2023
5 checks passed
@jahe jahe deleted the bugfix/tooltip-in-modal branch August 1, 2023 14:52
@github-actions
Copy link
Contributor

🎉 This PR is included in version 3.0.0-next.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

Tooltip causes scrollbar in Modal dialog
3 participants