ContextMenu fixes & Overlay didClose #2399
Conversation
fix Overlay outside clickPreview: documentation | landing | table |
| const onClose = spy(); | ||
| ContextMenu.show(MENU, { left: 0, top: 0 }, onClose); | ||
| it("invokes onClose callback when menu closed", done => { | ||
| ContextMenu.show(MENU, { left: 0, top: 0 }, done); |
There was a problem hiding this comment.
feels a little odd to me to do things this way w/o an assertion
There was a problem hiding this comment.
this is a pattern i see frequently in other test suites: it essentially asserts that the callback is called at all. if it's not called then it will time out.
There was a problem hiding this comment.
👍 seems reasonable, I didn't know the exact timeout behavior
| @@ -361,7 +368,7 @@ export class Overlay extends React.PureComponent<IOverlayProps, IOverlayState> { | |||
|
|
|||
| const isClickInThisOverlayOrDescendant = openStack | |||
| .slice(stackIndex) | |||
| .some(({ containerElement }) => containerElement && containerElement.contains(eventTarget)); | |||
| .some(({ containerElement: elem }) => elem && elem.contains(eventTarget) && !elem.isSameNode(eventTarget)); | |||
There was a problem hiding this comment.
leave a comment why we need the isSameNode check?
| @@ -62,10 +61,10 @@ class ContextMenu extends AbstractPureComponent<{}, IContextMenuState> { | |||
| onInteraction={this.handlePopoverInteraction} | |||
| position={Position.RIGHT_TOP} | |||
| popoverClassName={popoverClassName} | |||
| popoverDidClose={this.props.didClose} | |||
| target={<div />} | |||
There was a problem hiding this comment.
is it better to make this "div" so as not to break shallow comparison of props for updates? though you'd also need to change backdropProps above
There was a problem hiding this comment.
i don't understand the action item here. i could make it a static constant?
There was a problem hiding this comment.
or just make it "div", right?
There was a problem hiding this comment.
but either way, prop comparison will never be equal because of backdropProps above
There was a problem hiding this comment.
"div" is quite different from <div /> here... one would appear as three letters in the DOM, the other is an empty element.
There was a problem hiding this comment.
ah, I see, you have to use targetElementTag for that?
There was a problem hiding this comment.
targetElementTag is actually the parent target prop, and a target is required so you can't just set that prop
| }; | ||
| const TRANSITION_DURATION = 100; | ||
|
|
||
| /* istanbul ignore next */ | ||
| class ContextMenu extends AbstractPureComponent<{}, IContextMenuState> { | ||
| class ContextMenu extends AbstractPureComponent<{ didClose: () => void }, IContextMenuState> { |
There was a problem hiding this comment.
maybe interface instead of inline? not sure what current BP standards are
|
👍 everything generally looks reasonable to me. Just as a caveat, it's been a little while since I've looked at complicated BP code like |
ICMProps, commentPreview: documentation | landing | table |
Fixes #2131, Fixes #803, Fixes #1188
Changes proposed in this pull request:
several intertwined changes here with some very tricky associated logic. i added unit tests!
ContextMenumodifiers such that it won't leave the viewport (instead of window; fixes UX issue: Popovers near the bottom of the page often expand page height #803)OverlaydidCloseandPopoverpopoverDidCloselifecycle methods!react-transition-grouponExitedmethod (did not exist in 1.0)ContextMenuelement when the menu closes (using lifecycle method above; fixes No abbility to ContextMenu.show after modal dialog #2131)Overlayoutside click logic Adding Scrollable overlays breaks canOutsideClickClose with hasBackdrop false #1188