refactor(IText): extract draggable text logic to a delegate #8598
Conversation
fixes flicker while calling `setDragImage`
| @@ -101,7 +101,7 @@ type TDestroyedCanvas = Omit< | |||
| * flag = this.canDrop(opt.e); | |||
| * }); | |||
| * b.canDrop = function(e) { | |||
| * !flag && this.callSuper('canDrop', e); | |||
| * !flag && this.draggableTextDelegate.canDrop(e); | |||
There was a problem hiding this comment.
should I move this example to DraggableTextDelegate?
| * @returns {boolean} determines whether {@link target} should/shouldn't become a drop target | ||
| */ | ||
| canDrop(e: DragEvent): boolean { | ||
| if (this.target.editable && !this.target.__corner && !e.defaultPrevented) { |
There was a problem hiding this comment.
changed:
- if (this.target.editable && !this.target.__corner) {
+ if (this.target.editable && !this.target.__corner && !e.defaultPrevented) {| /** | ||
| * in order to respect overriding {@link IText#canDrop} we call that instead of calling {@link canDrop} directly | ||
| */ | ||
| protected targetCanDrop(e: DragEvent) { | ||
| return this.target.canDrop(e); | ||
| } |
| } | ||
|
|
||
| dragEnterHandler({ e }: DragEventData) { | ||
| const canDrop = this.targetCanDrop(e); |
There was a problem hiding this comment.
changed:
call targetCanDrop instead of canDrop
|
|
||
| dragOverHandler(ev: DragEventData) { | ||
| const { e } = ev; | ||
| const canDrop = this.targetCanDrop(e); |
ShaMan123
changed the title
| const offset = correction.add(diff).transform(vpt, true); | ||
| // prepare instance for drag image snapshot by making all non selected text invisible | ||
| const bgc = target.backgroundColor; | ||
| const styles = clone(target.styles, true); |
There was a problem hiding this comment.
note for merging:
#8600 will cause a merge conflict.
itext_behavior.mixin should be resolved to ours and:
| const styles = clone(target.styles, true); | |
| const styles = cloneDeep(target.styles); |
Don't forget to adjust imports!
There was a problem hiding this comment.
And the cumbersome draggableTextDelegate assignment can go away
|
|
||
| // TODO: replace this with a standard assignment when shitty `clone` is removed | ||
| Object.defineProperty(this, 'draggableTextDelegate', { | ||
| value: new DraggableTextDelegate(this), | ||
| configurable: false, | ||
| enumerable: false, | ||
| writable: true, | ||
| }); |
|
Moving to this now. |
| this.on('mousedown', this.onMouseDown); | ||
|
|
||
| // TODO: replace this with a standard assignment when shitty `clone` is removed |
There was a problem hiding this comment.
i'll truncate this comment before merging up to the appropriate part.
| * @public override this method to control whether instance should/shouldn't become a drop target | ||
| */ | ||
| canDrop(e: DragEvent) { | ||
| return this.draggableTextDelegate.canDrop(e); |
There was a problem hiding this comment.
why we would suggest to overrid the class and not create a custom draggable test delegate?
There was a problem hiding this comment.
both can be done...
I was thinking of your guideline of being able to control dnd with ease, so I felt it is best to have methods on the instance so the dev doesn't need to understand the delegate
| @@ -71,6 +71,8 @@ export const isFabricObjectWithDragSupport = ( | |||
| return ( | |||
| !!fabricObject && | |||
| typeof (fabricObject as FabricObjectWithDragSupport).onDragStart === | |||
| 'function' && | |||
| typeof (fabricObject as FabricObjectWithDragSupport).shouldStartDragging === | |||
There was a problem hiding this comment.
would be there a case in which onDragStart exists but not shouldStartDragging?
|
This PR is good as it is, but the draggableTextDelegate ( i this a good name ? ) should be somehow optional or we should have an empty one that allow for disabling the feature and not carrying te code around if not wanted. Probably a matter of an extra class extension and everyone can import text from the preferred export. |
Motivation
making IText logic more scoped and readable
Another step forward typing text
Description
After most of DnD bugs have been fixed in #8534 I have extracted all logic to an external delegate
Changes
The following have been extracted to
DraggableTextDelegate:__isDragging=> rename to__mouseDownInPlaceand exposed asDraggableTextDelegate#isActiveand asIText#shouldStartDraggingfor canvasChanges to draggable text logic:
canDrop8bcc8a6 6fcadb8:!e.defaultPreventedis now part of logic (all calls tocanDropwere prefixed with it)targetCanDropis called intead ofcanDropso overridingIText#canDropwill have an affectAdditional:
__lastSelectedto click behavior where it belongs (selectedshould be moved there as well)initBehaviorGist
First merge #8534 => fix-text-dnd...text-dnd-followup
In Action