search

bpmn-io / diagram-js

A toolbox for displaying and modifying diagrams on the web.
MIT License
1.65k stars 418 forks source link

feat(context-pad): position absolute intead of relative to element #888

Closed philippfromme closed 2 months ago

philippfromme commented 2 months ago
nikku commented 2 months ago

BREAKING CHANGE: context pad is not an overlay anymore; is positioned absolutely and does not scale

How is this a breaking change? What changes from the customization point of view?

philippfromme commented 2 months ago

Tests only fail on Linux with Firefox. There the position seems to be off. 🤔

philippfromme commented 2 months ago

@marstamm Could you try this out with Linux & Firefox to validate that the position for sequence flows actually works?

marstamm commented 2 months ago

I can reproduce the test failure locally on a non-headless firefox. The test does not have any visuals to compare but I can't see any difference between chrome and firefox when using it in bpmn-js.

I can spend some time investigating why it calculates the position differently, or we can just accept that it works despite this and skip the test conditionally

philippfromme commented 2 months ago

Confirmed to work for the user on Linux & Firefox. ✅

philippfromme commented 2 months ago

I tried to improve the deprecation notice. Ready for review.

nikku commented 2 months ago

I tested this (visually) against bpmn-js and observed that the positioning is slightly different:

New

image

Old

image

Is this intended or an issue with my setup?

philippfromme commented 2 months ago

I adjusted the position so the margin scales with the canvas zoom. So the context pad doesn't scale but the margin does just like with the original context pad. We're using the same approach with the other non-scaling non-overlay UI elements: https://github.com/camunda/improved-canvas/pull/61/files#diff-a6b19a26ee73a0b10318b24b4bfa2c36bfd0c32443f520a13af79ef3af2a0b8cR47

philippfromme commented 2 months ago

I will go ahead an merge this.