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

feat(canvas): allow creating planes from descriptors #599

Merged
merged 2 commits into from
Dec 8, 2021

Conversation

marstamm
Copy link
Contributor

@marstamm marstamm commented Nov 30, 2021

  • Allows us to reuse plane descriptors when undo/redoing plane removals.

supersedes #598

@marstamm marstamm requested a review from nikku November 30, 2021 15:51
@marstamm marstamm self-assigned this Nov 30, 2021
@bpmn-io-tasks bpmn-io-tasks bot added the needs review Review pending label Nov 30, 2021
@marstamm marstamm added in progress Currently worked on and removed needs review Review pending labels Dec 8, 2021
@marstamm
Copy link
Contributor Author

marstamm commented Dec 8, 2021

As discussed, we will change the API like this:

  • removePlane removes the root element from the elementRegistry but not from the plane descriptor
  • createPlane always adds the root element to the elementRegistry if it is present in the descriptor

Potentially Breaking Changes

  • createPlane only accepts plane descriptors. We align them more with elements, renaming the attribute name to id

Other potential improvements
#600

- add rootElement on createPlane
- remove rootElement on removePlane, but keep rootElement as part of planeDescriptor
@marstamm marstamm force-pushed the create-planes-from-descriptor branch from db76a1e to f4aa05b Compare December 8, 2021 13:31
@marstamm
Copy link
Contributor Author

marstamm commented Dec 8, 2021

Ready for review. I only adjusted with non-breaking changes and kept the general API for now.

An example of how it is used is implemented here: https://github.com/bpmn-io/bpmn-js/pull/1538/files#diff-72a5e1f4e48dd45f7e30a07fb4234e5978d62f9ac15fb2d716b1348d0503e758R37-R58

@marstamm marstamm added needs review Review pending and removed in progress Currently worked on labels Dec 8, 2021
children: [],
isImplicit: true
};
}

var svgLayer = this.getLayer(name, PLANE_LAYER_INDEX);
svgAttr(svgLayer, 'display', 'none');
this._addRoot(plane.rootElement, plane);
Copy link
Member

Choose a reason for hiding this comment

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

I like this very much, as if we're already transitioning to #600. 😉 ❤️

}

if (typeof plane === 'string') {
plane = { name: plane };
Copy link
Member

Choose a reason for hiding this comment

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

Could simplify this to:

if (typeof plane === 'string') {
  plane = { name: plane, rootElement: rootElement };
}

rootElement = {
id: '__implicitroot' + name,
if (!plane.rootElement) {
plane.rootElement = rootElement || {
Copy link
Member

Choose a reason for hiding this comment

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

Could simplfy this to:

plane.rootElement = { ... }

If we do ⬆️

@fake-join fake-join bot merged commit 195c8d0 into develop Dec 8, 2021
@fake-join fake-join bot deleted the create-planes-from-descriptor branch December 8, 2021 15:03
@bpmn-io-tasks bpmn-io-tasks bot removed the needs review Review pending label Dec 8, 2021
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 this pull request may close these issues.

2 participants