everweij / react-laag

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

Positioning issue #8

Closed rijk closed 5 years ago

rijk commented 5 years ago

Not sure what is going wrong here, but check out this sandbox: https://codesandbox.io/s/react-laag-tooltip-positioning-ej9hs

The tooltip is not positioned correctly:

image

Note that when you remove the <nav> component, somehow it works:

image

This took like 2 hours to reproduce.. hope you can figure it out 😅

rijk commented 5 years ago

Also note that the problem doesn’t go away when using container={document.body}. So it is not an error in placement of the tooltip, but really in calculating the trigger offset. It does work when using fixed, even when the tooltip is placed in the same container (<main> in this case).

everweij commented 5 years ago

Hi Rijk, ouch... I have taken a quick look and I immediately saw this warning message in the console:

react-laag: Set the 'position' style of the nearest scroll-container to 'relative', 'absolute' or 'fixed', or set the 'fixed' prop to true.
This is needed in order to position the layers properly.
Currently the scroll-container is positioned: "static". Visit https://react-laag.com/docs/#position-relative for more info. 
<main>…</main>

I added position; relative; to the scroll-container (main in this case), and that seems to fix the problem. When I removed overflow: auto; from the main element, it also seemed to position correctly.

A little background, and please tell me if this make sense: react-laag places the layers into the nearest scroll-container (main), by searching for overflow: auto; / overflow: scroll; properties in the tree, and will eventually fallback to document.body. This scroll-container's positions than become the reference-point for the layer's position. In other words, the layer is positioned relative to the nearest scroll-container. During development mode, react-laag checks whether the scroll-container was indeed positioned relative (or absolute / fixed). In the sandbox you've provided, the positions are calculated relative to the main element, but the layer is placed on the body. I measured a 60px difference to be exact, and that's the same height as the nav element.

Does adding position; relative; work for you?

rijk commented 5 years ago

OMG, really?? I can’t believe I didn’t see that! You must think I’m stupid now, haha.

Yes, position; relative; works like a charm, thanks.

Edit: well looking back, I do see now how I missed the warning:

image

But I must admit I wasn’t at my sharpest yesterday 😴

everweij commented 5 years ago

No not at all, don't worry! ;)

Yeah, I get buried in those 'cross-site' warnings to, so I don't blame you for missing the warning! Do you think an error message is more appropriate? It is kind of an error in the sense that the actual behavior will be different than the user intented... although it doesn't break your app completely 🧐

rijk commented 5 years ago

Good point. An error would have caught my eye I think, but I agree a warning is more appropriate here. Although.. Yeah it’s really something you want to fix in your app, because the positioning will likely be off. So you could call it an error in that sense, yeah.

I think React triggers errors for these kinds of things in dev mode (developer needs to address it), but not in production (doesn’t break the app). Maybe you could use a similar approach here?

(I had to reopen the issue to be able to leave a comment)

everweij commented 5 years ago

Yeah, I do feel that the developer should address it in dev mode, so I tend to at change console.warning to console.error. Will put that on the next release ;)

(Will close this issue for now, because the title ('Positioning issue') is a bit misleading)