refactor(cdk/overlay): remove logic which tracks excluded elements from outside clicks #19887
Conversation
googlebot
added
the
cla: yes
| /** | ||
| * Array of HTML elements clicking on which should not be considered as outside click | ||
| */ | ||
| excludeFromOutsideClick?: HTMLElement[] = []; |
There was a problem hiding this comment.
I think we talked about having this be a predicate function instead of an array of elements.
There was a problem hiding this comment.
@jelbourn we later agreed that we should just emit on the _outsidePointerEvents subject and have the user decide correct?
There was a problem hiding this comment.
Yeah, when Andy and I followed-up on the discussion in the team meeting, I remembered that the outsidePointerEvents stream is, well, a stream that people can filter themselves however they want, so adding a predication would literally just be adding a call to filter, so calling the API is the same amount of work as adding a filter yourself.
There was a problem hiding this comment.
@jelbourn thinking about it now, adding a predicate can stop overlays added earlier from being processed since the loop would just break based on the predicates return. I can implement the menu with either approach but I'm not sure if there's some edge cases where we may want an early break.
Edit: agreed to leave as is
andy9775
force-pushed
the
cdk-overlay-outside-click
branch
from
0949dba to
0ff7434
Compare
|
|
||
| overlayRef.attach(new ComponentPortal(TestComponent)); | ||
| button.parentNode!.removeChild(button); | ||
| overlayRef.dispose(); |
There was a problem hiding this comment.
Should these disposals happen in an afterEach() block, so they happen even if the test fails?
There was a problem hiding this comment.
Slightly tricky because different tests create a different number of overlays; it would probably be better to de-dupe the dispose calls, but I wouldn't hold up the PR on it since it was already like this
There was a problem hiding this comment.
@teflonwaffles I agree that it should be cleaned up, but I opted to keep with the structure of the existing tests rather than polluting this PR with a test refactor.
|
|
||
| overlayRef.attach(new ComponentPortal(TestComponent)); | ||
| button.parentNode!.removeChild(button); | ||
| overlayRef.dispose(); |
There was a problem hiding this comment.
Slightly tricky because different tests create a different number of overlays; it would probably be better to de-dupe the dispose calls, but I wouldn't hold up the PR on it since it was already like this
jelbourn
added
lgtm
action: merge
|
@googlebot ping |
andy9775
force-pushed
the
cdk-overlay-outside-click
branch
from
0ff7434 to
8aae895
Compare
andy9775
force-pushed
the
cdk-overlay-outside-click
branch
2 times, most recently
from
afc8141 to
236c2e1
Compare
…om outside clicks This removes the ability to track excluded elements from the overlay and only stops outside click event notifications if the click event occurred within an overlay element itself. The overlay should not track elements to be excluded from outside click - this should be up to the developer. This allows for custom closeout logic to be implemented by the developer leaving the cdk to be unopinionated.
andy9775
force-pushed
the
cdk-overlay-outside-click
branch
from
236c2e1 to
f824928
Compare
|
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
This removes the ability to track excluded elements from the overlay and only stops outside
click event notifications if the click event occurred within an overlay element itself. The
overlay should not track elements to be excluded from outside click - this should be up
to the developer. This allows for custom closeout logic to be implemented by the developer leaving
the cdk to be unopinionated.