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

[EuiPopover] Let 'anchorPosition' determine anchor position, regardless of screen real estate #3845

Closed
i-a-n opened this issue Jul 31, 2020 · 6 comments

Comments

@i-a-n
Copy link
Contributor

i-a-n commented Jul 31, 2020

Hi team, I noticed that EuiPopover wasn't respecting anchorPosition if it calculated that the content took up too much screen real estate. It simply treats anchorPosition as a preference, and overrides it when necessary. See the example in the documentation:

Screen Shot 2020-07-31 at 3 38 04 PM

However, the documentation also states:

The alignment and arrow on your popover can be set with the anchorPosition prop. These positions will not update based upon screen real estate and will stick to the positions you declare.

So we have a case of documentation-implementation mismatch. This was discussed on Slack in this thread.

We need to be able to position a popover panel at a certain position regardless of screen real estate. Is that a property you would consider implementing? We definitely do need it, although I certainly understand why you don't think we should be doing this. However, as we are mimicking a dropdown with an EuiInputPopover, there are many states—especially in mobile views—that look and feel awkward when they reposition themselves above the trigger.

Screen Shot 2020-07-31 at 4 15 35 PM

So we do think this is a feature that would help us out here quite a bit. Thanks for your time!

@cchaos
Copy link
Contributor

cchaos commented Aug 3, 2020

I'm not sure giving the ability to completely turn this positioning feature off is the correct path. There will always need to be some sort of affordance for the popover based on the window/body. For instance, if this example's input was actually at the bottom of the page, then forcing the popover to continue to drop down instead of up, would force extra scrolling and height onto the the <body> causing a page-jitter.

Instead, we should allow the container prop of EuiPopover to take precedence over the window edges. So that when passing the ref of the <body> as the container, it can gauge the actual bounds of that container and adjust and not just the window edges.

@i-a-n
Copy link
Contributor Author

i-a-n commented Aug 6, 2020

Thanks for taking the time @cchaos! Just to close the feedback loop here, using a container to calculate placement in addition to the window edge does sound like an improvement to me, but if I'm understanding correctly, still won't help us with our particular use-case.

Our popover has always been within the bounds of <body> anyway, so if window edges are still used to calculate position at all, it would still result in our popover appearing in a different place than the specified anchorPosition.

I'm personally 100% comfortable managing my UI's layout to avoid the scrollbar jank (which I think already happens with EuiModal) when an anchorPosition is specified. I was totally prepared to follow the documentation's advice here 😄 :

These positions will not update based upon screen real estate and will stick to the positions you declare. Because of this, be careful when using left or right positioning.

@chandlerprall
Copy link
Contributor

chandlerprall commented Aug 6, 2020

The container functionality right now, is if one is provided, the smaller bounds of container & window are used to determine the positioning. That is, if the container is 1000px wide but the window itself is only 500px, that window size overrides the container's.

The proposed change is to add a flag alongside the container to indicate that the window's size should not be used. So, given this flag + container=body, an anchorPosition=downCenter popover will expand down as long as there is room in the body, regardless of the window size & scroll position. It would still expand up if the anchor is at the bottom of its container.

@i-a-n
Copy link
Contributor Author

i-a-n commented Aug 6, 2020

@chandlerprall oh!, that's great, I didn't catch that part. that definitely sounds like it'd suit our needs much better. thanks for explaining.

@github-actions
Copy link

github-actions bot commented Sep 7, 2021

👋 Hey there. This issue hasn't had any activity for 180 days. We'll automatically close it if that trend continues for another week. If you feel this issue is still valid and needs attention please let us know with a comment.

@github-actions
Copy link

❌ We're automatically closing this issue due to lack of activity. Please comment if you feel this was done in error.

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

No branches or pull requests

4 participants