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: render markers as part of djs-visual #2173

Merged
merged 3 commits into from
May 28, 2024
Merged

feat: render markers as part of djs-visual #2173

merged 3 commits into from
May 28, 2024

Conversation

marstamm
Copy link
Contributor

@marstamm marstamm commented May 27, 2024

This allows styling with canvas.addMarker and css, as described in our examples.
Requires bpmn-io/diagram-js#906

related to camunda/camunda-modeler#4307

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 like this change; local markers (for coloring) seems to be a good thing to do, and it benefits the issue at hand (CSS styling), too.

Still need to E2E test this against a larger diagram. Good job 👏

@marstamm
Copy link
Contributor Author

I manually tested against complex.bpm (https://demo.bpmn.io/s/application-processing) and there was no noticeable performance decrease

@marstamm marstamm marked this pull request as ready for review May 28, 2024 12:40
@bpmn-io-tasks bpmn-io-tasks bot added needs review Review pending and removed in progress Currently worked on labels May 28, 2024
@marstamm marstamm requested review from a team, philippfromme and barmac and removed request for a team May 28, 2024 12:40
CHANGELOG.md Show resolved Hide resolved
@marstamm marstamm force-pushed the marker-rework branch 2 times, most recently from 32cfc2b to d8791a4 Compare May 28, 2024 13:05
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 don't assume that rendering of these cryptic markers was ever done via userland CSS, hence I regard these changes as not breaking.

Good job 🌵

@marstamm marstamm merged commit d09e3b8 into develop May 28, 2024
11 checks passed
@marstamm marstamm deleted the marker-rework branch May 28, 2024 13:14
@bpmn-io-tasks bpmn-io-tasks bot removed the needs review Review pending label May 28, 2024
@nikku
Copy link
Member

nikku commented May 28, 2024

@marstamm Please verify the diagram-js update does not break dmn-js, and if it does bump diagram-js there, too.

Please also cut a release of this change (in bpmn-js), so users can have a clean (and compatible) install again.

@marstamm
Copy link
Contributor Author

I verified that the diagram-js update does not break dmn-js. I'll release bpmn-js soon, but old bpmn-js versions are also compatible with the new diagram-js version.

Only reason some tests failed before the update was a CSS selector checking for a certain number of paths in a visual, which broke the assertion when we added the marker with an additional path. The behavior was still working as intended

@marstamm
Copy link
Contributor Author

I want to get #1676 (comment) into the release

@marstamm
Copy link
Contributor Author

Released as v17.8.0

@nikku nikku mentioned this pull request May 28, 2024
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.

2 participants