Bubble dragover and dragenter events #14
Conversation
The implementation for github/github#190219 needs an event to kick off finding the cursor position, and rather than bubbling the DragEvent it seems prudent to fire a CustomEvent solely used by that consumer. That'll avoid any unintended side effects in the document at large.
|
Given where the root PR is, I think this is ready for a proper look 🙇 |
src/file-attachment-element.ts
Outdated
| cancelable: true, | ||
| detail: { | ||
| dragEvent: event, | ||
| attachmentElement: target |
There was a problem hiding this comment.
What do you think about calling this target instead? attachmentElement made me instinctually think it was the thing being dragged, rather than the element we are dragging over
There was a problem hiding this comment.
That's a good distinction! 👍
There was a problem hiding this comment.
We've now removed the custom event entirely 🎉
src/file-attachment-element.ts
Outdated
| attachmentElement: target | ||
| } | ||
| }) | ||
| target.dispatchEvent(dragEvent) |
There was a problem hiding this comment.
We don't check if the event was cancelled. Do we need the event to be cancellable?
There was a problem hiding this comment.
@koddsson that's a really good question! Even though it doesn't need to be for https://github.com/github/github/pull/190219, since this event could be used for any number of things down the road being able to preventDefault() on it and then checking if the event was cancelled might be useful. I'll set cancelable: true
There was a problem hiding this comment.
Oh, Wednesday morning 🤦. It already is cancellable. Really I could go either way. I don't think we're going to regret having it be cancellable, but in truth I don't think we'll ever need to check if it's been cancelled. Probably less is more in this case, but I'll wait to hear back.
Bestra
force-pushed
the
bestra/file-attachment-dragged-event
branch
from
2e5a30b to
25a4242
Compare
Bestra
force-pushed
the
bestra/file-attachment-dragged-event
branch
from
25a4242 to
312836e
Compare
|
Based on an excellent question from @koddsson, I'm going to remove the custom event from this PR, and instead propagate the original |
We decided we could use the `dragover` and `dragenter` events directly instead of wrapping them with a custom one. We're still calling `preventDefault()` so we won't accidentally trigger any native behavior downstream.
Bestra
changed the title
What this PR does
Current direction
The implementation for https://github.com/github/github/pull/190219 needs
an event to kick off finding the cursor position, and after some discussion it makes the most sense to
bubble the
dragoveranddragenterevents that we're currently stopping.Originally
The implementation for https://github.com/github/github/pull/190219 needs
an event to kick off finding the cursor position, and rather than directly propagating the
DragEvent it seems prudent to fire a CustomEvent solely used by that consumer.
That'll avoid any unintended side effects in the document at large.