feat(create): allow create with keyboard #733
Conversation
|
@barmac @nikku could one of you have a look? Would love to get this in and released so I can move on with bpmn-io/bpmn-js#1802 |
| @@ -307,6 +308,10 @@ export default function Create( | |||
| }); | |||
| }); | |||
|
|
|||
| if (event instanceof KeyboardEvent) { | |||
There was a problem hiding this comment.
There are two options to go about this:
Make Create.start work out-of-the-box, implicitly using the mouse utility or explicitly invoke Create.start with a mouse event (that you got from wherever).
While I like your solution we did it differently in the past, cf.
.Did you consider the alternative?
There was a problem hiding this comment.
I didn't come across this in my deep dive unfortunately 😅 We can do this approach too and I guess it's more contained.
The approach I did adds it to the core and prevents us from having to think about it again. Seeing as it is something useful in general and not just this one case, it might make sense to go this way. As far as I could tell (me and the test suite 🤣), it didn't "ruin" anything else. But I can also see how it may look random in the middle of create.
I don't have a super strong opinion: if we would rather keep this on a need-to-use basis locally, i'm also happy would that.
There was a problem hiding this comment.
Let us consider it like this:
What value do I get from passing in a KeyEvent over a MouseEvent over null into create.start(...)? If there is none then I'd advocate to make it possible to call create.start without an event, defaulting to the last mouse move event, if provided.
If there is one, let's ensure that we don't break that.
There was a problem hiding this comment.
But I can also see how it may look random in the middle of create.
We could make the dependency to mouse an optional one (injector.get('mouse', false)) and clearly document that the last good known mouse position is chosen, if no event is provided.
There was a problem hiding this comment.
I don't have a super strong opinion: if we would rather keep this on a need-to-use basis locally, i'm also happy would that.
If this does not involve much work I'd favor the local solution, I think.
There was a problem hiding this comment.
Works for me. I will close this PR then and handle it locally.
Related to bpmn-io/bpmn-js-connectors-extension#54 and bpmn-io/bpmn-js#1801 (comment)