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

Force popover contents to stay in the same position side once open #1199

Conversation

chandlerprall
Copy link
Contributor

@chandlerprall chandlerprall commented Sep 18, 2018

Closes #1187

Summary

Previously, a popover could jump to a different position (top, left, right, bottom) when its contents changed or onScroll (if the repositionOnScroll prop was passed). This PR changes the behaviour to lock the popover to whatever position it initially opens to, unless the window is resized.

While it is locked to a side, the popover still re-positions to center content, keep the correct offset, and other position calculations - but it only calculates those for the now-forced position.

This is testable by editing the popover's DOM content in chrome dev tools.

Checklist

  • This was checked in mobile
  • This was checked in IE11
  • This was checked in dark mode
  • Any props added have proper autodocs
  • Documentation examples were added
  • A changelog entry exists and is marked appropriately
  • This was checked for breaking changes and labeled appropriately
  • Jest tests were updated or added to match the most common scenarios
  • This was checked against keyboard-only and screenreader scenarios

@cjcenizal
Copy link
Contributor

cjcenizal commented Sep 18, 2018

It looks like this is partially working, but there's an odd bug I encountered when I tested it in Kibana:

popover_jumping

This popover is positioned with downRight. When I first enter input into the search, the popover repositions which is unexpected. But the good thing is that it doesn't reposition after that.

@chandlerprall
Copy link
Contributor Author

@cchaos please test again with the context menu stuff, should be positioning correctly now

@cjcenizal should have your 2nd case (jumping on first input) fixed now

Copy link
Contributor

@cjcenizal cjcenizal left a comment

Choose a reason for hiding this comment

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

Nice work! Tested this locally and it looks like it's working.

popover_working

Code LGTM.

@cchaos
Copy link
Contributor

cchaos commented Sep 26, 2018

It's working very well, there are some edge cases when the trigger is near browser edges, but I think those can be fixed in implementations and don't need to be handled here.

Is there any way to possibly not visibly show the popover/context menu until it is in the new position?

There's a flash of it in the wanted position until it re-positions.

@chandlerprall
Copy link
Contributor Author

Thanks for testing again!

Is there any way to possibly not visibly show the popover/context menu until it is in the new position?

I'll ruminate on this for a bit.

@chandlerprall
Copy link
Contributor Author

@cchaos I've updated context menu's panel rendering to not transition/animate the initial panel. This positions the panel correctly on initial showing.

Copy link
Contributor

@cchaos cchaos left a comment

Choose a reason for hiding this comment

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

Oh heck yeah! It works and looks soo much better now. The animation on first open was bugging us.

@chandlerprall chandlerprall merged commit 6063ed3 into elastic:master Oct 3, 2018
@chandlerprall chandlerprall deleted the feature/1187-restrict-popover-position branch October 3, 2018 16:37
@snide snide mentioned this pull request Oct 3, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants