everweij / react-laag

Hooks to build things like tooltips, dropdown menu's and popovers in React
https://www.react-laag.com
MIT License
907 stars 47 forks source link

Support for portals? #5

Closed rijk closed 4 years ago

rijk commented 4 years ago

In the process of reimplementing my react-popper-based Tooltip component with react-laag, I ran into an issue. I’ve used Popper’s createPortal support to position my tooltips at a specific place in the DOM. Something like this:

<Tooltip
  tooltip="Project status"
  container={document.querySelector( '.grid .projects .tooltips' )}
>
  <Project />
</Tooltip>

There’s a couple reasons I do this:

  1. I can easily control the tooltips z-index by setting it on the .tooltips element;
  2. The width and style (font size, weight etc) of the tooltips is not influenced by its target;
  3. But more importantly, I can make sure the tooltips scroll with the content (without JS repositioning jank). I simply put the .tooltips container in the same scroll parent as the content and it automatically works.

When I attempt to use a portal in renderLayer, the positioning is off. Any advice or is this simply not supported?

everweij commented 4 years ago

Hi Rijk, that's a good question! I have thought about a couple of use cases, but it could very well be that I've overlooked a couple of them. I really like to research your use-case. Maybe we could achieve the same result with a different method that's already supported by the library, or maybe there should be an extra feature in the library covering this use-case. Right now, I'm on a little holiday, but I will get back at you asap!

everweij commented 4 years ago

I'm back from my little holiday :) I took same time to look at the points you've described, and I have a couple of questions:

  1. I can easily control the tooltips z-index by setting it on the .tooltips element;

You could give each layer an extra className of let say .tooltip. Then somewhere in your css you could give that class a general z-index with the value you like. Does this make sense?

  1. The width and style (font size, weight etc) of the tooltips is not influenced by its target;

That's a fair point. Normally, I try to avoid to depend on inherited styles, and rather try to set them explicitly, but I don't know if that will work for you.

  1. But more importantly, I can make sure the tooltips scroll with the content (without JS repositioning jank). I simply put the .tooltips container in the same scroll parent as the content and it automatically works.

I see your concern there. It's true that react-laag does some calculations based on a couple of events (ie: scrolling / resizing), but only when the layer is open. react-laag also only triggers a React re-render when the resulting style (injected into the layer) actually changes. Let say you have a layer that is positioned BOTTOM_CENTER that stays that way no matter what, it will not re-render for the next couple of seconds you scroll. So generally, there shouldn't be too much positional stuff going on.


In order to fully understand your use-case(s), is it possible for you to create a little minimal CodeSandbox with the react-popper / tooltip you've described? I could then make an attempt to port the same functionality to react-laag.

rijk commented 4 years ago

You could give each layer an extra className of let say .tooltip. Then somewhere in your css you could give that class a general z-index with the value you like.

Not quite; as the z-index of children can never exceed their parent’s, this won’t work if the parent already has relative positioning and a z-index (example). Unless you use position: fixed on the tooltip, but this is a very crude fix and you lose all ability to scroll.

I try to avoid to depend on inherited styles, and rather try to set them explicitly

The point is that I want to prevent inherited styles. You are right, it doesn’t happen often since most of the styles are already explicitly defined in my .tooltip class — but due to the cascading nature of CSS you can never fully rule out unwanted side effects by accidental style inheritance.

Injecting the tooltips in a container in a controlled location solves both these issues, and the scrolling issue, in a very elegant and simple way. When you place it just inside the overflow parent, it only has to be positioned once and scrolling the content will automatically scroll the tooltip; no repositioning needed, no delay, no jank.

This sentence in the docs actually got me excited:

By default react-laag tries to render the layer within the closest scroll-container.

because I thought that react-laag would traverse the DOM tree to look for the nearest parent with overflow: auto. That would actually be really cool behaviour if it was built-in (would 100% work for me), and would even make sense to make the default I think; you always want your tooltips to scroll with the trigger, right? And users already have the option to override it using fixed mode.

Right now, the tooltip is just inserted right at the location you placed it in the React tree. So in my opinion it’s not really as stated in the docs.

If auto-placing it in the overflow parent is not possible, react-laag should at least be able to work with Portals (or, alternatively, offer a container property where I can pass a DOM element) so I can control where the layer is inserted in the DOM.

Makes sense? :)

Will try to make a Sandbox today if I have time.

rijk commented 4 years ago

Here’s a Sandbox: https://codesandbox.io/s/react-laag-scrolling-tooltips-z2z4l

I’ve put the Step tooltips on click instead of hover, so they are easier to inspect. You can change this in Step.jsx by setting hover={true}. You will also see that I created a Tooltip component (in Tooltip2.jsx) with a pretty complex render function, that adds the listeners directly to the children so we don’t have to add a wrapper <div> or <span> (this will often lead to layout issues).

Issues:

image

everweij commented 4 years ago

Thanks for your extensive elaboration Rijk, it certainly makes sense! I see that the relative parents with their own z-indexes seem to be a problem indeed, and definitely need to be fixed.

By default react-laag tries to render the layer within the closest scroll-container.

I think a made a mistake here, or at least the wrong assumption. Right now, the layers are mounting inside the first relative parent. That makes sense when the overflow-container is the relative parent (which the most of my use-cases are), but your example shows that doesn't have to be the case at all. I would like to propose the following:

Anyway, I will create a new branch and will look for a solution. Will let you know if I have something ;)

I will make a new issue for the hover/scale effect in fixed mode (glad you found out).

rijk commented 4 years ago

Sounds great! I think if A were implemented well, you will almost never even need B. So it also sounds like a great fit for the philosophy of the library.

Regarding your concerns about B: react-popper supports using (your own) portals, and doesn’t really mention any pitfalls. I think I even tried using it once with a container outside the scroll-container — it worked fine but just had a little repositioning lag when scrolling. That’s why I eventually opted for specific tooltip containers inside each module’s scroll container, so the tooltips scroll with the content automatically, just looked a little more smooth. But it certainly worked in react-popper, they even mention it as a use case:

This can be useful if you want to position a tooltip inside an overflow: hidden container that you want to make overflow. Please note that you can also try the positionFixed strategy to obtain a similar effect with less hassle.

So maybe you can check that package out for inspiration.

Good luck!

everweij commented 4 years ago

Hi Rijk, I've made some adjustments. I've forked your example and bumped it to the latest version (1.2.0) and it seems to be working correctly now (https://codesandbox.io/s/react-laag-issue-nr5-v6pe1)

I've also made some small optimizations and added a container prop together with some documentation.

Could you let me know if this fixes the issue for you?

rijk commented 4 years ago

Wow, that’s quick! I’ll check it out. 👍

everweij commented 4 years ago

@Rijk, do you think I can close this issue? :)

rijk commented 4 years ago

Hm strange, I already replied to this issue but it must have not been saved, sorry about that!

It works like a charm, I just updated my codepen to 1.2.0 and it worked right away, great work. 👌

Only one comment, and I am nitpicking here: I noticed the wrapper divs for the tooltips are added to the dom straight away:

image

Do you think it would be possible to only add them when the tooltip is open?

everweij commented 4 years ago

No problem, I'm glad it worked out ok!

I did some quick tests, and I think I can dump the wrapper-divs entirely. That way, the layers will only be in the dom when necessary

everweij commented 4 years ago

I released v1.3.0 today 🎉 The wrapper div's should be gone now. I'm gonna close this issue for now, but please let me know if there's anything else!

rijk commented 4 years ago

Awesome. The package is getting better and better!