-
Notifications
You must be signed in to change notification settings - Fork 420
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
Remove Touch Interaction #845
Conversation
lib/features/connect/Connect.js
Outdated
@@ -122,7 +122,7 @@ export default function Connect(eventBus, dragging, modeling, rules) { | |||
/** | |||
* Start connect operation. | |||
* | |||
* @param {MouseEvent|TouchEvent} event |
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.
Why cannot this be a touch event, too? Let's ensure we're not removing the general ability to touch support.
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.
My idea was that all of these things could be added back in through an extension. But to validate that I'd have to build that extension. So for now I've reverted the changes.
lib/features/dragging/Dragging.js
Outdated
@@ -174,8 +167,7 @@ export default function Dragging(eventBus, canvas, selection, elementRegistry) { | |||
assign( | |||
{}, | |||
dragContext.payload, | |||
dragContext.data, | |||
{ isTouch: dragContext.isTouch } |
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.
See above, let's keep this (or layout alternatives).
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.
See #845 (comment)
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.
My question on this PR is:
- What options do contributors have to re-add touch support, given that we remove everything from the core? If there is some, then I'll be fine. If there is none, that essentially means we close the door to touch support completely, which blocks users from building a proper working touch support.
7a4b327
to
0d871fe
Compare
My idea was that all of these things could be added back in through an extension. But to validate that I'd have to build that extension. So for now I've reverted the changes. |
Let's merge these aspects. Gives users a chance to plug-in touch interaction (again). |
I will use it on mobile devices in my work, so how can I expand it if I want to add touch support now? Do you have your own examples of how to expand? |
Hi, please check out the examples repo: https://github.com/bpmn-io/bpmn-js-examples You will find there an example how to add a custom module to your bpmn-js project. Note that this needs to be done in your own app, and not at demo.bpmn.io. |
BTW I checked how demo.bpmn.io works on iPhone 13 Mini and it seems to be usable even after the changes. The only parts which don't work right now are the color picker and the append-anything menu. |
@barmac Are you sure it works? Because I tried the demo on both ipad and iphone and it doesn't work at all. You cannot pan, you cannot drag. Is there a work-in-progress re-implementation of touch support, as an extension, anywhere in the wild yet? |
There is no work-in-progress re-implementation yet, but if you'd like to contribute one we're happy to help. |
I tested now and only some interactions work on my iphone. I can place new elements, but cannot move the existing ones, and I cannot move around the diagram. I'd classify the tool as unusable on a touch-only device. |
Required by bpmn-io/bpmn-js#2079
Closes #796