[ClickAwayListener] Remove misleading code comment #20743
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
eps1lon
added
the
scope: click away listener
Details of bundle changes.Comparing: ceb7cda...75dd869 Details of page changes
|
|
Well, I didn't really test it beyond just googling at @oliviertassinari's request, but I saw things like
timmywil/panzoom@d4f132c#diff-6a0dde1466e98b2125be19c4d7cd5077 also shows a refactoring of The comment is there to explain why we're not doing the more simple |
|
#20406 did not change how we call |
No, but we were trying to get the bundle size down and trying to think of ways we could do it. I suggested
I then dug up those links above, and he said
I agree with that, should be able to drop. |
Agree, it's worth trying to remove it out #20197 (comment) insideDOM =
- !(doc.documentElement && doc.documentElement.contains(event.target)) ||
+ !doc.documentElement.contains(event.target) || |
|
Maybe the comment could be updated to say something like // This check could've been made simpler with
// `doc.contains` (works in modern browsers),
// but it isn't supported in IE 11:
// https://github.com/timmywil/panzoom/issues/450
// https://github.com/videojs/video.js/pull/5872 |
oliviertassinari
force-pushed
the
chore/cleanup-code
branch
from
104926b to
75dd869
Compare
oliviertassinari
approved these changes
Apr 28, 2020
NMinhNguyen
approved these changes
Apr 28, 2020
NMinhNguyen
reviewed
Apr 28, 2020
| } else { | ||
| const doc = ownerDocument(nodeRef.current); | ||
| // TODO v6 remove dead logic https://caniuse.com/#search=composedPath. | ||
| // `doc.contains` works in modern browsers but isn't supported in IE 11: |
There was a problem hiding this comment.
I guess there's no point mentioning doc.contains at all because this branch will be replaced entirely with event.composedPath in v6, and it's unlikely someone would try to drop documentElement from the code
This was referenced May 6, 2020
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
In that code
.containsis always called onHTMLElementnever onDocument.HTMLElement.prototype.containsis supported down to IE9. The linked PRs are not concerned withHTMLElement.prototype.containsbutDocument.prototype.contains.@NMinhNguyen Could you clarify what the comment meant?