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.

Sign up for GitHub

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(create): allow create with keyboard #733

Closed
wants to merge 1 commit into from
Closed

feat(create): allow create with keyboard #733

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 8 additions & 2 deletions lib/features/create/Create.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,8 @@ export default function Create(
dragging,
eventBus,
modeling,
rules
rules,
mouse
) {

// rules //////////
Expand Down Expand Up @@ -307,6 +308,10 @@ export default function Create(
});
});

if (event instanceof KeyboardEvent) {
Copy link
Member

Choose a reason for hiding this comment

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

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.

diagram-js/lib/features/copy-paste/CopyPaste.js

Line 242 in 9df8848

this._create.start(this._mouse.getLastMoveEvent(), elements, {
.

Did you consider the alternative?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

@nikku nikku Jan 17, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Works for me. I will close this PR then and handle it locally.

event = mouse.getLastMoveEvent();
}

dragging.init(event, PREFIX, {
cursor: 'grabbing',
autoActivate: true,
Expand All @@ -324,7 +329,8 @@ Create.$inject = [
'dragging',
'eventBus',
'modeling',
'rules'
'rules',
'mouse'
];

// helpers //////////
Expand Down
4 changes: 3 additions & 1 deletion lib/features/create/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ import DraggingModule from '../dragging';
import PreviewSupportModule from '../preview-support';
import RulesModule from '../rules';
import SelectionModule from '../selection';
import Mouse from '../mouse';

import Create from './Create';
import CreatePreview from './CreatePreview';
Expand All @@ -12,7 +13,8 @@ export default {
DraggingModule,
PreviewSupportModule,
RulesModule,
SelectionModule
SelectionModule,
Mouse
],
__init__: [
'create',
Expand Down
26 changes: 26 additions & 0 deletions test/spec/features/create/CreateSpec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -184,6 +184,32 @@ describe('features/create - Create', function() {
}));


it('should create from keyboard', inject(function(create, dragging, elementRegistry) {

// given
var parentGfx = elementRegistry.getGraphics('parentShape');
var keyboardEvent = new KeyboardEvent('keydown', { keyCode: 'Enter' });
document.dispatchEvent(keyboardEvent);

// when
create.start(keyboardEvent, newShape);

dragging.hover({ element: parentShape, gfx: parentGfx });

dragging.move(canvasEvent(getMid(parentShape)));

dragging.end();

// then
var createdShape = elementRegistry.get('newShape');

expect(createdShape).to.exist;
expect(createdShape).to.equal(newShape);

expect(createdShape.parent).to.equal(parentShape);
}));


it('should update elements and shape after create', inject(
function(create, dragging, elementFactory, elementRegistry, eventBus) {

Expand Down