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

Jump to bottom

Two-Step Pasting #390

Merged
merged 4 commits into from
Aug 7, 2019
Merged

Two-Step Pasting #390

merged 4 commits into from
Aug 7, 2019

Conversation

philippfromme
Copy link
Contributor

@philippfromme philippfromme commented Jul 30, 2019 •
edited
Loading

@bpmn-io-tasks bpmn-io-tasks bot added the in progress Currently worked on label Jul 30, 2019
@philippfromme philippfromme mentioned this pull request Jul 30, 2019
Merged
@philippfromme philippfromme changed the title WIP Two-Step Pasting Two-Step Pasting Aug 1, 2019
@nikku nikku added needs review Review pending and removed in progress Currently worked on labels Aug 1, 2019 — with bpmn-io-tasks
@philippfromme philippfromme force-pushed the 1421-two-step-pasting branch 3 times, most recently from 52b3c78 to 8c20819 Compare August 2, 2019 08:19
@nikku nikku added this to the M30 milestone Aug 2, 2019
@philippfromme philippfromme changed the title Two-Step Pasting WIP Two-Step Pasting Aug 2, 2019
@philippfromme philippfromme added in progress Currently worked on and removed needs review Review pending labels Aug 2, 2019 — with bpmn-io-tasks
@philippfromme philippfromme force-pushed the 1421-two-step-pasting branch 2 times, most recently from 263ec00 to e924d4a Compare August 2, 2019 10:00
@philippfromme philippfromme changed the title WIP Two-Step Pasting Two-Step Pasting Aug 2, 2019
@philippfromme
Copy link
Contributor Author

Ready to be reviewed.

@philippfromme philippfromme added needs review Review pending and removed in progress Currently worked on labels Aug 2, 2019 — with bpmn-io-tasks
nikku
nikku reviewed Aug 2, 2019
View reviewed changes
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.

Let's talk about the rationale behind some of these changes next week. I'd like to understand stuff before giving my 👍.

lib/features/copy-paste/CopyPaste.js Show resolved Hide resolved
lib/features/mouse/Mouse.js Outdated Show resolved Hide resolved
test/spec/features/mouse/MouseSpec.js Show resolved Hide resolved
lib/util/CopyPasteUtil.js Outdated
export function getParents(elements) {

// find elements that are not children of any other elements
return filter(elements, function(element) {
Copy link
Member

Choose a reason for hiding this comment

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

This implementation is super slow as it will run elements.length * elements.length times.

Does it have to be fast? If yes, let's look into 🚤 it up.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, that might be an issue, let's look into it. 💪

@philippfromme philippfromme mentioned this pull request Aug 2, 2019
Closed
@philippfromme philippfromme force-pushed the 1421-two-step-pasting branch 5 times, most recently from 8a5e27e to 66abc83 Compare August 4, 2019 12:34
@philippfromme philippfromme removed the needs review Review pending label Aug 5, 2019 — with bpmn-io-tasks
@philippfromme philippfromme added the in progress Currently worked on label Aug 5, 2019 — with bpmn-io-tasks
pinussilvestrus
pinussilvestrus reviewed Aug 5, 2019
View reviewed changes
Copy link
Contributor

@pinussilvestrus pinussilvestrus left a comment

Choose a reason for hiding this comment

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

Just some things I noticed

lib/features/create/Create.js Show resolved Hide resolved
lib/features/modeling/cmd/CreateElementsHandler.js Show resolved Hide resolved
lib/features/mouse/Mouse.js

// helpers //////////

export function createMoveEvent(x, y) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Cool stuff, maybe we can use it to improve the situation in the dirty HoverFixSpec . 👍

@philippfromme philippfromme force-pushed the 1421-two-step-pasting branch 6 times, most recently from 9d470de to 9b4bd6e Compare August 6, 2019 19:56
@philippfromme
feat(create): create multiple elements
8d7d1d9 
* create#start can be called with a single element or multiple elements
* create will assign x and y to shapes
* create will center elements around cursor
* create preview moved to CreatePreview
* PreviewSupport#addDragger add optional third argument gfx
* Modeling#createElements added

BREAKING CHANGES

* Create: hints are not passed to CreateShapeHandler anymore
* Create#start: third argument is context, if you want to specify source do { hints: { source: myElement } }
@philippfromme
feat(copy-paste): start create on paste
1ef5b34 
* CopyPaste#paste starts create by default
* add createElementsBehavior hint to prevent behavior on creating elements

BREAKING CHANGES

* CopyPaste: <elements.copied>, <element.copy>, <elements.copy>, <element.paste>, <elements.paste> removed, <copyPaste.canCopyElements>, <copyPaste.copyElement>, <copyPaste.elementsCopied>, <copyPaste.pasteElement>, <copyPaste.pasteElements> added
* Modeling#pasteElements removed in favor of Modeling#createElements
* MouseTracking removed in favor of Mouse
nikku
nikku reviewed Aug 7, 2019
View reviewed changes
lib/features/create/Create.js


/**
* Adds the ability to create new shapes via drag and drop.
*
Copy link
Member

Choose a reason for hiding this comment

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

Documentation is gone now, will we restore it some day, maybe? 👓

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If the code is not self-explanatory enough, yes. 🙉

@nikku nikku merged commit 1ef5b34 into develop Aug 7, 2019
@bpmn-io-tasks bpmn-io-tasks bot removed the in progress Currently worked on label Aug 7, 2019
@delete-merged-branch delete-merged-branch bot deleted the 1421-two-step-pasting branch August 7, 2019 13:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nikku nikku nikku left review comments

@pinussilvestrus pinussilvestrus pinussilvestrus left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
M30
Development

Successfully merging this pull request may close these issues.
3 participants
@philippfromme @nikku @pinussilvestrus