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

Add append to contextPad #1801

Closed
2 tasks
Tracked by #1627
smbea opened this issue Jan 10, 2023 · 13 comments · Fixed by #1802
Closed
2 tasks
Tracked by #1627

Add append to contextPad #1801

smbea opened this issue Jan 10, 2023 · 13 comments · Fixed by #1802
Assignees

Comments

@smbea
Copy link
Contributor

smbea commented Jan 10, 2023

What should we do?

Add a "append anything" entry to the palette which provides a menu with all possible elements.

What it should look like:

To do:

  • Add entry to context pad
  • Add append menu entries provider

Why should we do it?

To support create anything, see #1627

@smbea smbea changed the title [bpmn-js] Add append to contextPad Add append to contextPad Jan 10, 2023
smbea added a commit that referenced this issue Jan 10, 2023
@bpmn-io-tasks bpmn-io-tasks bot added the in progress Currently worked on label Jan 10, 2023
smbea added a commit that referenced this issue Jan 11, 2023
smbea added a commit that referenced this issue Jan 11, 2023
@bpmn-io-tasks bpmn-io-tasks bot added needs review Review pending and removed in progress Currently worked on labels Jan 11, 2023
smbea added a commit that referenced this issue Jan 12, 2023
smbea added a commit that referenced this issue Jan 12, 2023
@smbea smbea added the in progress Currently worked on label Jan 12, 2023 — with bpmn-io-tasks
@smbea smbea removed the needs review Review pending label Jan 12, 2023
smbea added a commit that referenced this issue Jan 13, 2023
@smbea smbea added the needs review Review pending label Jan 13, 2023 — with bpmn-io-tasks
@smbea smbea removed the in progress Currently worked on label Jan 13, 2023
@bpmn-io-tasks bpmn-io-tasks bot added in progress Currently worked on and removed needs review Review pending labels Jan 13, 2023
@smbea
Copy link
Contributor Author

smbea commented Jan 16, 2023

UX discussion outcome (2023-01-16):

Let's interpret "append anything" as "add anything", a tool that can basically replace the palette in a way that is always close to where you are modeling. It provides all elements you might need. We can see how users react to this and adapt if needed.

  • We shall keep elements that can't be directly connected (like event sub processes and boundary events).
  • For boundary events: make sure boundary events aren't transformed into intermediate events. onClick should also trigger create mode (same as onDragStart).
  • An improvement that can be done later on is to automatically attach boundary events to the tasks.

@smbea
Copy link
Contributor Author

smbea commented Jan 16, 2023

Currently having some difficulty with triggering create mode on keyboard (same as bpmn-io/bpmn-js-connectors-extension#54). The dragging module is not prepared to receive a KeyboardEvent because it can't extract the cursor position from it, therefore not being able to calculate the initial dragging position.

I only got this far (needs an initial mouse movement to be able to display the hover element):

Screen.Recording.2023-01-16.at.16.37.43.mov

@barmac
Copy link
Member

barmac commented Jan 16, 2023

How about we fake the initial cursor position based on the button position? That could mimic the mouse interaction. Still, it looks good enough as we don't aim for keyboard-only modeling at the moment.

@smbea
Copy link
Contributor Author

smbea commented Jan 16, 2023

@barmac I tried to calculate the position based on the target but it wasn't working out too well. We also try to get this position again (here) after the menu has been closed to add the placeholder, so we would need to save this position and re-use it. I did some messing around with it but the positioning was always off.

@nikku
Copy link
Member

nikku commented Jan 16, 2023

We have a utility to record and provide the last mouse position Mouse#getLastMoveEvent.

I think it is fine to use that (and otherwise resort to "show only once mouse moved for the first time).

@nikku
Copy link
Member

nikku commented Jan 16, 2023

Also interesting if #1801 (comment) does not work: Why does it work so smoothly in the prototype?

@smbea
Copy link
Contributor Author

smbea commented Jan 17, 2023

Following our UX discussion, there are still some other things we could clear up:

  • Behaviour of catch events vs (interrupting) boundary events: since we allow changing between boundary events to intermediate catch events, should we keep an element for each? It may be redundant but I think it's fine to keep both since the triggered action (autoPlace, create mode) is different.
Screen.Recording.2023-01-17.at.14.56.25.mov
  • Previously, we were removing the append option from the context pad for elements that can't have outgoing connections (see ongoing PR) but since we changed our perspective on append, should we keep the option to append? Maybe makes sense for event subprocess. Does it make sense for end event?
Screen.Recording.2023-01-17.at.15.03.33.mov

@nikku
Copy link
Member

nikku commented Jan 17, 2023

Behaviour of catch events vs (interrupting) boundary events

I'm aware of that case and would keep it as is.

Maybe makes sense for event subprocess. Does it make sense for end event?

We could go either way about this, but we should be consistent: Either we show the menu for all flow elements (end, event-sub + others) or only others. Intuitively I'd not show for both (end, event-sub) for now.

Also: Should we show it for elements where the context pad is conclusive? Event-based gateway is such a candidate.

@smbea
Copy link
Contributor Author

smbea commented Jan 17, 2023

Also: Should we show it for elements where the context pad is conclusive? Event-based gateway is such a candidate.

I'd say we can keep it initially and we can do some refining after the initial PR (#1802) has been merged. Wdyt?

@barmac
Copy link
Member

barmac commented Jan 17, 2023

I was surprised to learn that we cannot add text annotation via the append menu. Perhaps that could make sense even for event subprocesses and end events.

@nikku
Copy link
Member

nikku commented Jan 17, 2023

I was surprised to learn that we cannot add text annotation via the append menu. Perhaps that could make sense even for event subprocesses and end events.

Let's log this as a follow up, too. Makes sense to annotate from there, too.

@barmac
Copy link
Member

barmac commented Jan 17, 2023

I was surprised to learn that we cannot add text annotation via the append menu. Perhaps that could make sense even for event subprocesses and end events.

Let's log this as a follow up, too. Makes sense to annotate from there, too.

Created as #1806

smbea added a commit that referenced this issue Jan 18, 2023
smbea added a commit that referenced this issue Jan 18, 2023
smbea added a commit that referenced this issue Jan 18, 2023
@bpmn-io-tasks bpmn-io-tasks bot added needs review Review pending and removed in progress Currently worked on labels Jan 18, 2023
smbea added a commit that referenced this issue Jan 18, 2023
smbea added a commit that referenced this issue Jan 18, 2023
smbea added a commit that referenced this issue Jan 18, 2023
smbea added a commit that referenced this issue Jan 18, 2023
@bpmn-io-tasks bpmn-io-tasks bot removed the needs review Review pending label Jan 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants