throttle/debounce user callback after preventDefault/stopPropagation is executed #3481
Conversation
This file contains 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
ekwoka
approved these changes
Mar 24, 2023
| } | ||
| if (modifiers.includes('throttle')) { | ||
| let nextModifier = modifiers[modifiers.indexOf('throttle')+1] || 'invalid-wait' | ||
| let wait = isNumeric(nextModifier.split('ms')[0]) ? Number(nextModifier.split('ms')[0]) : 250 |
There was a problem hiding this comment.
This isn't yours, but I think just
Number(nextModifier.replace('ms','')) || 250
would be more efficient and get the same job done.
There was a problem hiding this comment.
good catch, reads better too. I've made the change and confirmed existing tests cover this
|
@hudon thanks for the PR! Can you revert your last commit? I think that should be changed in a separate PR, that way if we have to revert this PR (or the new PR) if any changes cause issues, then we're not having to revert everything. Thanks! |
|
@joshhanley done |
|
@hudon sweet, thanks! |
|
This makes sense to me as a needed fix. Thanks! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
This fixes the issue outlined here: #3478
There is a tradeoff worth noting here and perhaps a breaking change. This fix implies that if a user types
dragover.prevent.throttle, they do intend the dragover events to all preventDefault(), even the ones that end up not firing the callback because they're getting throttled. If some users however are depending on the current behavior and expecting the entire wrapped callback to be throttled (stopProp/preventDef included), this patch will break their use case. It would be inconsistent behavior, though, to have the event sometimes propagate and sometimes not.repost of #3479 from feature branch