-
Notifications
You must be signed in to change notification settings - Fork 4.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
useDropZone: Ensure drag event targets HTMLElement #34272
Conversation
Attempts to fix a TypeError that has been logged in the wild but is yet to be reproduced. In production builds, it manifests as `e.dataset is undefined`, pointing to the useDropZone hook. * Don't cast FocusTarget instance to HTMLElement prior to calling isElementInZone. * Perform type validations inside isElementInZone, then cast to HTMLElement.
Size Change: +25 B (0%) Total Size: 1.04 MB
ℹ️ View Unchanged
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, just a small nitpick 😄
) { | ||
return false; | ||
} | ||
|
||
/** @type {HTMLElement|null} */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can it still be null
? It looks like the previous if
statement would rule that possibility out.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good question! The answer is yes, there needs to be margin for null
for the subsequent do/while loop. Otherwise, TS complains at elementToCheck = elementToCheck.parentElement
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, I see, I missed that it's a let
and not a const
. 👍
Thanks for the quick review, @talldan! |
Attempts to fix a TypeError that has been logged in the wild but is yet to be reproduced. In production builds, it manifests as
e.dataset is undefined
, pointing to the useDropZone hook. In addition:How has this been tested?
Should have no measure impact on regular usage.
Types of changes
Code quality. Tentative bug fix.
Checklist:
*.native.js
files for terms that need renaming or removal).