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

Use bpmn in color #1453

Merged
merged 3 commits into from
May 27, 2021
Merged

Use bpmn in color #1453

merged 3 commits into from
May 27, 2021

Conversation

barmac
Copy link
Member

@barmac barmac commented May 26, 2021

Along with custom bpmn.io extension properties, modeling#setColor will use BPMN in Color properties to serialize element colors.

@bpmn-io-tasks bpmn-io-tasks bot added the needs review Review pending label May 26, 2021
@barmac
Copy link
Member Author

barmac commented May 26, 2021

Follow-up: #1454

@barmac
Copy link
Member Author

barmac commented May 26, 2021

Follow-up issue: #1455

@barmac barmac marked this pull request as draft May 26, 2021 10:55
@bpmn-io-tasks bpmn-io-tasks bot added in progress Currently worked on and removed needs review Review pending labels May 26, 2021
@barmac barmac force-pushed the use-bpmn-in-color branch 2 times, most recently from b5d4809 to a71a8bd Compare May 26, 2021 11:09
@barmac barmac marked this pull request as ready for review May 26, 2021 11:09
@bpmn-io-tasks bpmn-io-tasks bot added needs review Review pending and removed in progress Currently worked on labels May 26, 2021
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.

I'd personally not introduce any breaking changes at this point. Is it something we could avoid? A way to do it could be:

  • Detect if bioc is already used, if so, serialize using our color format.
  • If bpmn-in-color is already used, serialize in bpmn-in-color format.
  • If non is used, serialize in bioc because that is (at least at the moment) widely understood by bpmn-js viewers out there

Once our bpmn-in-color support received wider adoption we could consider to go the full switch

@barmac
Copy link
Member Author

barmac commented May 26, 2021

I'd personally not introduce any breaking changes at this point. Is it something we could avoid?

I think the most important breaking change is that we start to throw errors in setColor. Alternatively, we could just emit a warning and ignore incorrect values.

Regarding using bioc, if I understand your proposal correctly, this would mean we use BPMN in Color only for the diagrams where it has been used already. So if a user wants to use it, they would have to manually add the namespace in the XML or use another tool which supports the extension. I see a problem with this solution that it IMO violates the no magic principle:

  • If I create a diagram with a 3rd party tool and add colors, they will work in bpmn-js.
  • If I create a diagram with bpmn-js and add colors, they won't work in a 3rd party tool.
  • It can also be that I create a diagram with a 3rd party tool, add colors in bpmn-js and then they also won't work work in the 3rd party tool.

@barmac
Copy link
Member Author

barmac commented May 26, 2021

We've discussed this offline and come up with the following:

  • The stricter validation is not considered a breaking change but rather a fix. The invalid values have not been displayed incorrectly anyway.
  • We will use both bpmn.io custom properties and BPMN in Color in the setter. This way, at a cost of small redundancy, the old viewers will still be able to display colors modelled in new versions of the library.

@barmac
Copy link
Member Author

barmac commented May 26, 2021

OK I've removed the breaking change.

lib/Modeler.js Outdated Show resolved Hide resolved
@nikku
Copy link
Member

nikku commented May 27, 2021

Looks good. Pending integration of bpmn-moddle with BPMN in color support baked in.

@barmac
Copy link
Member Author

barmac commented May 27, 2021

Looks good. Pending integration of bpmn-moddle with BPMN in color support baked in.

Done via 7d58d6a

@nikku
Copy link
Member

nikku commented May 27, 2021

Did not seem to work according to tests @barmac.

And it seems like we did not properly inline the bpmn-in-color definition into the released bpmn-moddle library: https://unpkg.com/browse/bpmn-moddle@7.1.0/dist/index.js

@nikku
Copy link
Member

nikku commented May 27, 2021

Needs follow up fix in bpmn-moddle: bpmn-io/bpmn-moddle#88

@barmac
Copy link
Member Author

barmac commented May 27, 2021

Yea. I'll release bpmn-moddle@7.1.1 and itegrate it.

@barmac
Copy link
Member Author

barmac commented May 27, 2021

Looks good now 😓

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.

Good work. Happy we got this one sorted 👏

@fake-join fake-join bot merged commit 439bc4e into develop May 27, 2021
@fake-join fake-join bot deleted the use-bpmn-in-color branch May 27, 2021 12:43
@bpmn-io-tasks bpmn-io-tasks bot removed the needs review Review pending label May 27, 2021
@falko
Copy link
Member

falko commented Jun 25, 2021

Awesome! When will this be available in Camunda Modeler?

@nikku
Copy link
Member

nikku commented Jun 25, 2021

Will be released with the next release.

@falko
Copy link
Member

falko commented Jun 30, 2021

When will it be included in the nightly builds so we can start using it for the MIWG work? Is there another GitHub issue that I can follow for that?

@barmac
Copy link
Member Author

barmac commented Jun 30, 2021

It's already included in the nightly :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants