Conversation
jfirebaugh
requested review from
anandthakker,
lucaswoj,
mollymerp and
mourner
anandthakker
left a comment
anandthakker
left a comment
There was a problem hiding this comment.
💯 this is an excellent refactor.
@jfirebaugh what's your thinking re: *move events: does it still make sense for handlers to manage listening to them directly, or should we have the bindHandlers listeners dispatch to the appropriate handlers for these like we're doing for *{start,end}, etc.? (I can see why we might not want to have *move support default prevention, but I'm wondering whether it might still be worthwhile to have all events go through a unified set of listeners.)
|
|
||
| map.on('mousemove', onMove); | ||
| map.once('mouseup', onUp); | ||
| }); |
There was a problem hiding this comment.
👏 this is such a nice simplification, and (therefore, IMO) confirms this API design.
src/ui/bind_handlers.js
Outdated
| originalEvent: e | ||
| })); | ||
| function fireTouchEvent(type: string, originalEvent: TouchEvent) { | ||
| map.fire(new MapTouchEvent(type, map, originalEvent)); |
There was a problem hiding this comment.
Let's just inline these
| * | ||
| * * On `mousedown` events, the behavior of {@link DragPanHandler} | ||
| * * On `mousedown` events, the behavior of {@link DragRotateHandler} | ||
| * * On `mousedown` events, the behavior of {@link BoxZoomHandler} |
There was a problem hiding this comment.
I think we should also have:
- On
wheelevents, the behavior of{@link ScrollZoomHandler}
| if (e.touches) { | ||
| if ((e.touches: any).length > 1) return; | ||
| window.document.addEventListener('touchmove', this._onMove); | ||
| window.document.addEventListener('touchmove', this._onMove, {capture: true}); |
There was a problem hiding this comment.
Include comment mentioning why capture: true
The latter was what I tried initially, but I ended up reverting. The issue is that the drag handlers need to capture move events by binding the listener to the window (for the duration of the drag), while the general move event handler should receive only canvas container move events (but is permanent). So it's easier to keep them separated. |
|
Looks great @jfirebaugh! Thank you for improving the clarity & robustness of our event type system while working on this new feature. |
mollymerp
left a comment
mollymerp
left a comment
There was a problem hiding this comment.
this looks great! thanks for all the new tests, too!! 🥇
Changes the arguments of Evented#fire from (string, Object) to (Event) and updates all call sites. This paves the way for events to provide preventDefault(). It also allows Error subclasses to enforce conventions via types. Here, we enforce that the `error` property of error events is an Error, and correct several instances where it was a string instead.
jfirebaugh
force-pushed
the
preventDefault
branch
from
3e240a9 to
e2ba66c
Compare
6 participants
Tested in Mac Chrome, Mac Firefox, iOS Safari.