[Overlay] Support nested overlays #2033
Conversation
[Overlay] Support nested overlays. Fixes #2030Preview: documentation | landing | table |
|
looks like you need more containers 😅
|
|
Are builds like this not allowed in free CircleCI accounts? That's unfortunate... we'll have to refactor the test workflow steps |
|
|
||
| const isClickInOverlay = | ||
| (this.containerElement != null && this.containerElement.contains(eventTarget)) || | ||
| isClickInDescendantOverlay; |
There was a problem hiding this comment.
don't || here cuz it makes the variable name incorrect (includes a check for clicks not in overlay). || in the if.
| .slice(stackIndex) | ||
| .map(findDOMNode) | ||
| .filter(elem => elem && elem.contains) | ||
| .some(elem => elem.contains(eventTarget)); |
There was a problem hiding this comment.
👌 sweet array manipulation
| const stackIndex = openStack.indexOf(this); | ||
|
|
||
| const isClickInDescendantOverlay = openStack | ||
| .slice(stackIndex) |
There was a problem hiding this comment.
i'm struggling to wrap my head around why this works... new ("descendant") Overlays are pushed to the end of the stack so we can slice off everything before this one?
|
@tyscorp got a moment to update this PR? if not, please close it and i'll pick up this feature. |
tyscorp
force-pushed
the
nested-overlays
branch
from
c263f3d
to
47de1ca
Compare
|
Thanks for the reminder. I updated with the requested changes. I'm not sure how to write a unit test for this. |
[Overlay] Support nested overlays. Fixes #2030Preview: documentation | landing | table |
|
@tyscorp thanks! in the future, please do not amend/force-push commits--it destroys the links to outdated changes (try clicking on filename in one of my outdated comments above): https://github.com/palantir/blueprint/pull/2033/files/c263f3d136f5884357d65597524f386610bc1095#diff-567c0c7f15281ce63ededd202d7220f5 |
|
@tyscorp I'm going to need a test before I can accept this sort of change. make a test that asserts that a simple repro, ahem, doesn't reproduce after this change. take the code that led you to this bug, simplify it to just use |
tyscorp
force-pushed
the
nested-overlays
branch
from
ccc2da2
to
688596d
Compare
Added testPreview: documentation | landing | table |
Fixed lintPreview: documentation | landing | table |
|
|
||
| const isClickInDescendantOverlay = openStack | ||
| .slice(stackIndex) | ||
| .map(findDOMNode) |
There was a problem hiding this comment.
If we change the visibility of Overlay#containerElement to public, then we can avoid this findDOMNode call. I would prefer that approach.
| const isClickInOverlay = this.containerElement != null && this.containerElement.contains(eventTarget); | ||
| if (isOpen && this.props.canOutsideClickClose && !isClickInOverlay) { | ||
|
|
||
| if (isOpen && this.props.canOutsideClickClose && !isClickInOverlay && !isClickInDescendantOverlay) { |
There was a problem hiding this comment.
Since isClickInDescendantOverlay includes this overlay instance (openStack.slice(stackIndex) will include this Overlay), I think we can remove isClickInOverlay altogether.
| const stackIndex = openStack.indexOf(this); | ||
|
|
||
| const isClickInDescendantOverlay = openStack | ||
| .slice(stackIndex) |
code style tweaksPreview: documentation | landing | table |
|
thanks @tyscorp! |
Fixes #2030
Checklist
Changes proposed in this pull request:
Uses
Overlay.openStackto test if a document click is in a descendant overlay. Children overlays should always be after the parent's index, so I think this should work for most cases.This PR does not handle the case of having a non-
OverlayBlueprint.PortalorReactDOM.createPortal.