feat(auto-place): scroll canvas to new element #1437
Conversation
| var bbTrbl = asTRBL(boundingBox); | ||
|
|
||
| // Bigger padding to accommodate the context pad | ||
| var paddingRight = 80; |
There was a problem hiding this comment.
where do we get this value from?
There was a problem hiding this comment.
It's the css width of the context-pad (72px) + additional padding
There was a problem hiding this comment.
Ok got it, thanks for the clarification 👍
There was a problem hiding this comment.
I also discussed it with @philippfromme and we decided to make the padding consistent (80 to all sides) to cover most use-cases
| var viewBox = { x: 100, y: 100, width: 100, height: 100 }; | ||
|
|
||
| // when | ||
| var scrollArguments = getScrollOffset(element, viewBox); |
There was a problem hiding this comment.
So we are testing the util, but not the actual event handler? Is this intended?
| var viewBox = canvas.viewbox(); | ||
| var shape = event.shape; |
There was a problem hiding this comment.
Just some personal preference: let's try to stick with our current style
var viewBox = canvas.viewBox(),
shape = event.shape;also maps to other examples below
| * Always have the new element in frame | ||
| */ | ||
| export function getScrollOffset(element, boundingBox) { | ||
| var elementTrbl = asTRBL(element); |
There was a problem hiding this comment.
cf. above, I'd stick with the current style of multi-variable declaration
|
|
||
|
|
||
| /** | ||
| * BPMN auto-place behavior. | ||
| * | ||
| * @param {EventBus} eventBus | ||
| */ | ||
| export default function AutoPlace(eventBus) { | ||
| export default function AutoPlace(eventBus, canvas) { |
There was a problem hiding this comment.
can you maybe explain, why this is a bpmn-js but no generic diagram-js concern?
There was a problem hiding this comment.
One could imagine this is a general create behavior. Elements created are always moved into sight. Or what exactly do you refer to here @pinussilvestrus?
There was a problem hiding this comment.
I see the benefits of moving this PR to diagram-js, since we also moved the AutoPlace behavior there with bpmn-io/dmn-js#470
This would also add this feature to DMN modeling, which makes sense to me
There was a problem hiding this comment.
So we have two things here
a) is it a diagram-js concern? I'd vote for yes
b) is this an auto-place (only) behavior? Good question, I'd expect that this also happens on creating any kind of element, no matter when this happens (auto-place, creating from the palette, ...)
There was a problem hiding this comment.
AFAIK, auto-place is the only way we can currently place elements outside of the viewbox. When creating elements from the pallet, the drag operation automatically scrolls the canvas already
There was a problem hiding this comment.
@marstamm We could argue that is the case, even though we do allow anyone to model via API: https://cdn.statically.io/gh/nikku/2021-token-simulation/v0.0.2/presentation.html#22
Used by some of our side projects, e.g. https://github.com/bpmn-io/bpmn-js-cli and https://github.com/nikku/bpmn-js-cli-modeling-dsl.
We could argue that auto place is the natural higher level API that ships with such behavior. People and robots (cf. WASDENN) are free to implement this this in a similar fashion.
There was a problem hiding this comment.
One idea: Can we make this "ensure visible" / scrollToNode a thing on the canvas in diagram-js? Scroll to an element is a common use case people ask us about on our forums. It is also being used in our search feature already. Moving it into a shared place and exposing it as an API would allow a broader user base to benefit.
There was a problem hiding this comment.
Great Idea, let's move it into the canvas 👍
There was a problem hiding this comment.
@nikku , I'm not sure if we should replace the behavior in the search feature, as the current implementation is "center on screen" and not "scroll until visible". On the other hand, changing the behavior would closer resemble a text search, as no scrolling is happening when the element is already visible. What do you think?
There was a problem hiding this comment.
On the other hand, changing the behavior would closer resemble a text search, as no scrolling is happening when the element is already visible.
I think it makes a lot of sense to scroll a little bit less. The search result display is already very verbose. So 👍 from my perspective to scroll until visible only.
We may also consider to add a little bit more padding around the to be visible element. It does make sense to show it with context (i.e. what is behind). Would appreciate your input / if you experiment with it a little bit.
|
closed in favor of bpmn-io/diagram-js#545 |
Automatically scrolls the added element into view
related to camunda/camunda-modeler#1249