Refactor EuiPopover to use new positioning service #948

chandlerprall · 2018-06-27T19:33:29Z

Closes #873

Refactored EuiPopover to use portals and take advantage of the positioning service. The service had to be expanded to include an align parameter to allow shifting content into a given corner if there's room.

snide · 2018-06-27T23:12:38Z

Biggest thing I saw here was that i'd be nice if we can recalculate position on window resize. Right now if you resize the page while the popover is open, the popover retains its original position.

I don't consider this a blocker, and given a choice between our current functionality where this thing breaks and doesn't work in overflows, I'd rather see a follow up PR (if we think it would take awhile to fix) so that the main fix gets in quickly.

Either way I'd assume @cchaos or I would do a design PR as as a fast followup to fix the arrow coloring issue.

cjcenizal

Code LGTM. I had a few small suggestions but I also noticed some bugs on the ContextMenu page.

I expected the first example to move above the button when the window was very short, but instead it stays below the button and gets clipped:

The last example appears over the button, even when there seems to be enough room below it:

cjcenizal · 2018-06-27T19:41:27Z

src/components/popover/popover.js

+    let arrowStyles = {};
+    let arrowPosition = null;
+    if (node != null) {
+      // panel is coming into existance


Typo: existence

cjcenizal · 2018-06-27T23:48:16Z

src/components/popover/popover.js

+  'left': 'left'
+};
+function getPopoverPositionFromAnchorPosition(anchorPosition) {
+  const [, primaryPosition] = anchorPosition.match(/^(.*?)[A-Z]/);


I'm not going to lie... I had to run this through regex101 to figure out what it was doing and understand how we're mapping anchor position to popover position.

I both like and dislike this code because it's clever. 😄 But I would really find something like this much more understandable:

const anchorPositionToPopoverPositionMap = { 'upCenter': 'top', 'upLeft': 'top', 'upRight': 'top', 'downCenter': 'bottom', 'downLeft': 'bottom', 'downRight': 'bottom', 'leftCenter': 'left', 'leftUp': 'left', 'leftDown': 'left', 'rightCenter': 'right', 'rightUp': 'right', 'rightDown': 'right', }; function getPopoverPositionFromAnchorPosition(anchorPosition) { return anchorPositionToPopoverPositionMap[anchorPosition]; }

WDYT?

I disagree about it being clever; I did consider doing something like this, but my preference for some kind of parsing vs. a lookup mapping is because the target information isn't correlated to the input, it is itself encoded in the input. Parsing it out enforces that relationship. I'll add a comment to each function describing what the regex is doing.

and I'll include tests for both!

cjcenizal · 2018-06-27T23:50:40Z

src/components/portal/portal.js

@@ -22,9 +16,6 @@ export class EuiPortal extends Component {
    } = this.props;

    this.portalNode = document.createElement('div');
-  }
-
-  componentDidMount() {


cjcenizal · 2018-06-27T23:56:12Z

Might be worth auditing the other examples for cases where popover is being used and making sure they look right.

snide · 2018-06-28T00:33:33Z

Wierdly, I'm seeing times where the clicks on context menu items also dismiss the popover.

cjcenizal

After talking this over with Chandler, I understand his perspective but I still disagree. That said, I don't feel strongly about this point so I give this code my LGTM. 😄

cjcenizal · 2018-06-28T21:25:54Z

src/components/popover/popover.js

+  // extract the first positional word from anchorPosition:
+  // starts at the beginning (" ^ ") of anchorPosition and
+  // captures all of the characters (" (.*?) ") until the
+  // first capital letter (" [A-Z] ") is encountered


I think the "why" (instead of "what") would be very helpful here too. Something like this:

// Pull out the part of the anchorPosition string that indicates cardinal position, // e.g. "upCenter" -> "center", "leftUp" -> "up".

cchaos · 2018-06-28T21:43:48Z

One thing I noticed real quick, is the positioning is still based on the .euiPopover versus the actual triggering element (button).

snide · 2018-06-28T21:56:23Z

@cchaos that's on us. It existed in the old one too. Has to do with the extra div. We can fix that in a follow up.

chandlerprall · 2018-06-28T22:00:16Z

@cjcenizal the ContextMenu issues are related to that component's timing in rendering content vs. positioning the popover. When the popover is created, the context panel has a height of only 2 pixels, so the initial placement is correct. Then more content comes in which screws everything up. I'm going to pull this out into a separate issue because of that component's complexity, and I expect it to grow into a more general approach for handling dynamic child content.

cchaos · 2018-06-28T22:03:59Z

src/components/popover/popover.js

    }
  };

+  buttonRef = node => this.button = node;


What's this for? I only see it being added a new div wrapper around the actual button.

It's used to identify the bounding box of the anchor button (line 177)

Ahh, ok the problem is that it's a div so it's block-level by default. We'll need to add display: inline-block to it.

cchaos · 2018-06-28T22:14:09Z

One more thing, if the page is horizontally scrolling it doesn't account for the scroll position:

chandlerprall · 2018-06-28T22:33:50Z

@cchaos I've added inline-block to the wrapping anchor div

…ow resize

chandlerprall · 2018-06-29T16:03:49Z

@cjcenizal fixed the problem where interacting with the popover would close it; issue was that the click outside detector wasn't respecting content rendered through a child portal, so I modified how the click detection works. Please review these changes & comments.

snide

With the context menu fix I'm good with merging this. Tested it in multiple browsers.

cjcenizal

@chandlerprall Great solution to the onClickOutside problem! Code LGTM. Thanks for the very clear explanation in the comments.

chandlerprall requested review from snide, cchaos and cjcenizal June 27, 2018 19:33

cjcenizal reviewed Jun 27, 2018

View reviewed changes

cjcenizal approved these changes Jun 28, 2018

View reviewed changes

cchaos reviewed Jun 28, 2018

View reviewed changes

chandlerprall mentioned this pull request Jun 28, 2018

EuiContextMenu doesn't position correctly in constrained container #955

Closed

chandlerprall added 8 commits June 29, 2018 10:00

Refactor EuiPopover to use portals, dynamic arrow positioning

98b9011

Popover positioning with alignent; fixed EuiPopover animations

7db9ecd

Apply arrow buffer to aligned content

c982ab8

Remove unused property

9304050

PR feedback: regex documentation, testing; reposition popover on wind…

d9dae7a

…ow resize

make EuiPopover's wrapping anchor div inline-block

01725e3

Update EuiOutsideClickDetector to think with portals

28a8026

changelog

4215a8d

chandlerprall force-pushed the feature/enhanced-popover branch from 607f0a9 to 4215a8d Compare June 29, 2018 16:02

snide approved these changes Jun 29, 2018

View reviewed changes

Merge branch 'master' into feature/enhanced-popover

29b8a36

cjcenizal approved these changes Jun 29, 2018

View reviewed changes

chandlerprall merged commit de26d2a into elastic:master Jun 29, 2018

chandlerprall deleted the feature/enhanced-popover branch June 29, 2018 19:59

cjcenizal mentioned this pull request Aug 8, 2018

[EuiContextMenu] Tabbing regressions #1101

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor EuiPopover to use new positioning service #948

Refactor EuiPopover to use new positioning service #948

chandlerprall commented Jun 27, 2018

snide commented Jun 27, 2018

cjcenizal left a comment

cjcenizal Jun 27, 2018

cjcenizal Jun 27, 2018

chandlerprall Jun 28, 2018

chandlerprall Jun 28, 2018

cjcenizal Jun 27, 2018

cjcenizal commented Jun 27, 2018

snide commented Jun 28, 2018

cjcenizal left a comment

cjcenizal Jun 28, 2018

cchaos commented Jun 28, 2018

snide commented Jun 28, 2018

chandlerprall commented Jun 28, 2018

cchaos Jun 28, 2018

chandlerprall Jun 28, 2018

cchaos Jun 28, 2018

cchaos commented Jun 28, 2018

chandlerprall commented Jun 28, 2018

chandlerprall commented Jun 29, 2018

snide left a comment

cjcenizal left a comment

Refactor EuiPopover to use new positioning service #948

Refactor EuiPopover to use new positioning service #948

Conversation

chandlerprall commented Jun 27, 2018

snide commented Jun 27, 2018

cjcenizal left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cjcenizal commented Jun 27, 2018

snide commented Jun 28, 2018

cjcenizal left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cchaos commented Jun 28, 2018

snide commented Jun 28, 2018

chandlerprall commented Jun 28, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cchaos commented Jun 28, 2018

chandlerprall commented Jun 28, 2018

chandlerprall commented Jun 29, 2018

snide left a comment

Choose a reason for hiding this comment

cjcenizal left a comment

Choose a reason for hiding this comment