Repository navigation
Restore DomEvent.off(el) functionality when calling with one arg - #7125
Merged
Merged
Conversation
johnd0e
marked this pull request as ready for review
May 5, 2020 13:51
johnd0e
marked this pull request as draft
May 5, 2020 16:10
johnd0e
force-pushed
the
6013_domevent_off
branch
4 times, most recently
from
May 6, 2020 12:44
64d9a01 to
6d81342
Compare
johnd0e
marked this pull request as ready for review
May 6, 2020 12:52
2 of 3 tasks
2 tasks done
johnd0e
force-pushed
the
6013_domevent_off
branch
3 times, most recently
from
March 18, 2021 12:29
662a567 to
2d2f3e2
Compare
johnd0e
force-pushed
the
6013_domevent_off
branch
from
November 1, 2021 10:37
2d2f3e2 to
60a7448
Compare
Falke-Design
requested changes
Nov 1, 2021
Falke-Design
left a comment
Falke-Design
left a comment
Member
There was a problem hiding this comment.
This will be very helpful!
| export function on(obj, types, fn, context) { | ||
|
|
||
| if (typeof types === 'object') { | ||
| if (types && typeof types === 'object') { |
Member
There was a problem hiding this comment.
Should we add a check if(!types) because now it will go into Util.splitWords(types); and throws an error (if types == undefined)
Collaborator
Author
There was a problem hiding this comment.
and throws an error (if types ==
undefined)
Because that is exactly what is desired - to catch user bug.
Member
|
Same comment as in another PR — should we avoid using |
Falke-Design
reviewed
Nov 1, 2021
Comment on lines
+59
to
+66
| function batchRemove(obj, filterFn) { | ||
| for (var id in obj[eventsKey]) { | ||
| var type = id.split(/\d/)[0]; | ||
| if (!filterFn || filterFn(type)) { | ||
| removeOne(obj, type, null, null, id); | ||
| } | ||
| } | ||
| } |
Member
There was a problem hiding this comment.
I suggest to move this function below off() because else the documentation can be irritating
To prevent bugs when call such code with undefined `types` arg: `L.DomEvent.off(el, types, ...)` Note: `typeof null` is `object`, that's why need additional check for this case
// @Alternative // @function off(el: HTMLElement, types: String): this // Removes all previously added listeners of given types.
It was not only unneeded but even harmful, as it could mask real bugs.
johnd0e
force-pushed
the
6013_domevent_off
branch
from
November 5, 2021 12:15
60a7448 to
aa7695e
Compare
Collaborator
Author
@mourner |
mourner
approved these changes
Nov 5, 2021
8 tasks done
2 of 3 tasks
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
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.
Currently we have code (unsuccessfully) trying to handle this case, see #6013 (comment)
This PR (originally based on #6032) fixes this case.
offadded:More strict arguments check to catch common bugs
All cases are covered by tests.
This change is