Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
feat(IText): Draggable text #7802
feat(IText): Draggable text #7802
Changes from 52 commits
4a00d88
d6d1eae
672f945
0298b3b
c7a39a0
f5957c8
d4bdf7f
a77f365
51c06ae
a04e57d
04ee04f
afedfc2
fc00090
7dfa629
934dd3c
f70651c
3d32720
34bc7b5
7ce1e7d
8d500b3
de9ca36
aa8c371
0efa15a
4f457b9
4d516ea
76d2a4d
cbe456c
6047c29
3df67f2
784a27a
17b3f9a
1ae3043
8fdf77b
d112f3b
57e81a1
63bbdfe
1b640f5
a2d2225
48e2dde
eef6895
34b5686
f6f701a
10a43d9
4642784
8e69763
4d441f3
e78310a
f6d1311
04f379a
f9ed862
c257c91
72b9a77
df3f7f2
efdd61e
97cebb1
ab637ac
fe7cc4d
8b06dfe
1b12168
6b8a1d9
49aa851
cfa7445
6033478
33d1cd6
f7ed62c
46206b1
cdf9abd
a9ca01e
de44140
c8b413b
2069128
c8c10f0
513199f
d610383
9ebdc62
0d89e2b
9734207
c181512
29456b1
4058520
61c96fa
ea9c099
829bc67
43afb2e
fd8515c
ecbf266
bd2b6c3
a875dfe
bc4e4c1
186d26d
d4fa70c
23f450f
c60716d
0ebb4bb
61a48f7
a6b5aa1
51e15fe
d07d6d3
be4cf22
d6f9c8f
b38b34c
2db76ec
f00e547
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
i would prefer to call this
_onDragStartTextInternal
( or similar ) and short circuit if the active object type does not inherit from text rather than checking if the activeObject has onDragStart function.If more drag/drop functionality get added in the future, each functionality can have its own handler and they can be turned off easily by not registering the event handler or de-registering it.
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.
I disagree because this way any object can utilize this logic out of the box.
The place to disable the event is from
onDragStart
- this is from the spec.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.
I don't want to open the doors to drag things that we didn't plan to support.
We are not adding text drag support, with a skeleton of infrastructure to do maybe more dragging, but we don't really know.
It is easier to add later than rollback
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.
Do you agree with the naming
onDragStart
?It is meant for override same as
onSelect
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.
don't forget to account for viewport transform
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.
also, would it not be better to keep the dragImage in the original resolution (with retinascaling) and just resize the drag image's dom element so that it stays in high res?
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.
Please elaborate
resizing by setting
width
height
and applying scale?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.
I tested on a high dpi display. The drag image is at a lower dpi than the canvas.
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.
I have seen that as well.
Any suggestions? Code?
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.
just spent some time looking into this... your code looks correct, I'm really not sure what's causing the dragimage to have a lower res 😐
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.
it is not retina scaled because there is probably no css for the drawImage.
indeed here https://github.com/fabricjs/fabric.js/pull/7802/files#diff-af5c45c9018fd92f35ba43f5e25303b5fe3b627d0f81d2d78b328746fe015b4eR468-R473 we are dividing the natural size of the image by the retina factor, making it smaller.
Not sure if the dragged image can targeted by a selector, or can be wrapped in a div and added a class that would retina-scale it.
Is not that much important we can try later
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.
If i remember correctly, when handling drag/drop with react.js i remember the dragged element can be anything. I believe you can remove the division by retina scaling and add a div around the canvas that will divide the size by 2 with css and so get the hiDpi effect.
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.
@melchiar try manipulating the offset value
Perhaps firefox accounts for retina scaling?