Skip to content
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

Merged
merged 2 commits into from
Jan 22, 2024
Merged

Remove Touch Interaction #845

merged 2 commits into from
Jan 22, 2024

Conversation

philippfromme
Copy link
Contributor

@philippfromme philippfromme commented Jan 16, 2024

Required by bpmn-io/bpmn-js#2079
Closes #796

@@ -122,7 +122,7 @@ export default function Connect(eventBus, dragging, modeling, rules) {
/**
* Start connect operation.
*
* @param {MouseEvent|TouchEvent} event
Copy link
Member

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.

Copy link
Contributor Author

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.

@@ -174,8 +167,7 @@ export default function Dragging(eventBus, canvas, selection, elementRegistry) {
assign(
{},
dragContext.payload,
dragContext.data,
{ isTouch: dragContext.isTouch }
Copy link
Member

@nikku nikku Jan 16, 2024

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).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member

@nikku nikku left a 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.

@nikku nikku marked this pull request as draft January 19, 2024 10:58
@bpmn-io-tasks bpmn-io-tasks bot added in progress Currently worked on and removed needs review Review pending labels Jan 19, 2024
@philippfromme
Copy link
Contributor Author

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.

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.

@philippfromme philippfromme marked this pull request as ready for review January 22, 2024 09:42
@bpmn-io-tasks bpmn-io-tasks bot added needs review Review pending and removed in progress Currently worked on labels Jan 22, 2024
@nikku
Copy link
Member

nikku commented Jan 22, 2024

Let's merge these aspects. Gives users a chance to plug-in touch interaction (again).

@philippfromme philippfromme merged commit c06bc47 into develop Jan 22, 2024
12 checks passed
@philippfromme philippfromme deleted the remove-hammerjs branch January 22, 2024 14:15
@bpmn-io-tasks bpmn-io-tasks bot removed the needs review Review pending label Jan 22, 2024
@tony2y
Copy link

tony2y commented Mar 6, 2024

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?
@philippfromme @nikku

@tony2y
Copy link

tony2y commented Mar 6, 2024

image Why can the demonstration address be moved after switching to mobile mode, and what do I need to do to achieve the same effect?

@barmac
Copy link
Member

barmac commented Mar 6, 2024

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.

@barmac
Copy link
Member

barmac commented Mar 6, 2024

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.

@vesper8
Copy link

vesper8 commented Jun 6, 2024

@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?

@nikku
Copy link
Member

nikku commented Jun 6, 2024

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.

@barmac
Copy link
Member

barmac commented Jun 7, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Drop hammerjs dependency
5 participants