[DO NOT MERGE] feat: allow single pointer drag#53
[DO NOT MERGE] feat: allow single pointer drag#53CecoMilchev wants to merge 3 commits intodevelopfrom
Conversation
| cancel?: Function; | ||
| mouseOnly?: boolean; | ||
| clickMoveClick?: boolean; |
There was a problem hiding this comment.
Some suggetions for naming changes are posted in the spec:
#52 (comment)
| // 300ms is the usual mouse interval; | ||
| // // However, an underpowered mobile device under a heavy load may queue mouse events for a longer period. | ||
| const IGNORE_MOUSE_TIMEOUT = 2000; | ||
| const ESC = 27; |
There was a problem hiding this comment.
We better extract this to an enum-like entity like KeyCode:
Other keys can be added to this enum and it can be reused.
| const { which } = e; | ||
|
|
||
| if ((which && which > 1) || this._ignoreMouse) { | ||
| bind(this.document, "contextmenu", preventDefault); |
There was a problem hiding this comment.
We better add a comment on why this is necessary (e.g. add info that we need the right mouse button for canceling the drag action).
It is also worth it to consider if there is a very very specific case if you stop dragging on something, but expect to open the context menu, e.g. if customers have added a ContextMenu component and expect it to be opened.
| @@ -92,12 +96,31 @@ export class Draggable { | |||
| const { which } = e; | |||
|
|
|||
| if ((which && which > 1) || this._ignoreMouse) { | |||
There was a problem hiding this comment.
Note that event.which is marked as deprecated, we can think about alternatives like https://developer.mozilla.org/en-US/docs/Web/API/MouseEvent/buttons and https://developer.mozilla.org/en-US/docs/Web/API/MouseEvent/button
| this.cancelDrag(e); | ||
|
|
||
| this._cancelHandler(e); |
There was a problem hiding this comment.
It seems like we better reverse the order and act depending on whether the dragCancel event is prevented or not - consider the scenario where consumers of the package are notified for the dragCancel event, but they prevent it - then the dragCancel event should allow not cancelling the dragCancel event, thus the cancelDrag function should not be executed.
The above is only true if we actually want to allow preventing events, which I don't think is currently supported in the kendo-draggable package. Still, it is still valid to think if reversing the order is better as we notify for the occurrence of the dragCancel and then we actually cancel it. It will also help if we introduce something like dragCancelEventArgs.preventDefault someday.
| if (e.keyCode === ESC) { | ||
| this.cancelDrag(e); | ||
| this._cancelHandler(e); | ||
| } |
There was a problem hiding this comment.
It seems like this behavior can be an option - navigable: true.
We discussed this offline - some suites like Kendo Angular/React might want to have their own events or specific navigation and want to cancel the drag on Escape optionally.
| unbind(this.document, "contextmenu", preventDefault); | ||
| } | ||
|
|
||
| this._isDragging = !this._isDragging; |
There was a problem hiding this comment.
| this._isDragging = !this._isDragging; | |
| this._isDragging = false; |
| if (!this._clickMoveClick) { | ||
| bind(this.document, "mouseup", this._mouseup); | ||
| bind(this.document, "mousemove", this._mousemove); | ||
| bind(this.document, "contextmenu", preventDefault); |
There was a problem hiding this comment.
This event wasn't prevented in the original version (and will prevent the "contextmenu", which might be interpreted as a regression) - it seems better not to have it here, but only when the lick-move-click dragging option is enabled. Am I missing something?
| if (this._isDragging) { | ||
| bind(this.document, "mouseup", this._mouseup); | ||
| } else { | ||
| bind(this.document, "mousemove", this._mousemove); |
There was a problem hiding this comment.
We can add a comment about why isDragging can be true here - in the general case, it is not possible for the "start" event to be triggered after a "move" action has taken place, but we want exactly this here, so an explanation would be helpful.
| bind(this.document, "pointerup", this._pointerup); | ||
| bind(this.document, "pointercancel", this._pointerup); | ||
| bind(this.document, "contextmenu", preventDefault); | ||
| bind(this.document, "pointermove", this._pointermove); | ||
| this._pressHandler(e); | ||
| } else { | ||
| if (this._isDragging) { | ||
| bind(this.document, "pointerup", this._pointerup); | ||
| bind(this.document, "pointercancel", this._pointerup); | ||
| } else { | ||
| bind(this.document, "pointermove", this._pointermove); | ||
| bind(this.document, "keydown", this._keydown); | ||
| this._pressHandler(e); |
There was a problem hiding this comment.
It seems to me that it is easy for us to attach more than 1 event handler here for the same event as we have some logic for binding and unbinding events.
As we do not want to have more than 1 handler - what about unbinding the event in the bind function and re-attaching it? This should save us from potential duplications - it would have been nice if we can check if the event has been attached already, but this will require more code.
CecoMilchev
changed the title
2 participants
Related to: #52
cc: @telerik/kendo-dev-leads