Skip to content
20 changes: 20 additions & 0 deletions src/actions/mover.ts
Original file line number Diff line number Diff line change
Expand Up @@ -144,6 +144,8 @@ export class Mover {
this.unpatchWorkspace(workspace);
this.unpatchDragStrategy(info.block);
this.moves.delete(workspace);
// Delay scroll until after block has finished moving.
setTimeout(() => this.scrollCurrentBlockIntoView(workspace), 0)
return true;
}

Expand Down Expand Up @@ -175,6 +177,8 @@ export class Mover {
this.unpatchWorkspace(workspace);
this.unpatchDragStrategy(info.block);
this.moves.delete(workspace);
// Delay scroll until after block has finished moving.
setTimeout(() => this.scrollCurrentBlockIntoView(workspace), 0)
return true;
}

Expand All @@ -197,6 +201,7 @@ export class Mover {
);

info.updateTotalDelta();
this.scrollCurrentBlockIntoView(workspace);
return true;
}

Expand All @@ -218,6 +223,7 @@ export class Mover {
info.totalDelta.y += y * UNCONSTRAINED_MOVE_DISTANCE * workspace.scale;

info.dragger.onDrag(info.fakePointerEvent('pointermove'), info.totalDelta);
this.scrollCurrentBlockIntoView(workspace);
return true;
}

Expand Down Expand Up @@ -307,6 +313,20 @@ export class Mover {
this.oldDragStrategy = null;
}
}

/**
* Scrolls the current block into view if exists one.
*
* @param workspace The workspace to get current block from.
*/
private scrollCurrentBlockIntoView(workspace: WorkspaceSvg) {
Copy link
Contributor

Choose a reason for hiding this comment

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

What are your thoughts on including an extra padding parameter in this method that defaults to 0?

I noticed when using constrained movement I wanted the scroll to make a bit more of the surrounding context visible to understand where I'm moving the blocks to. One way to do that would be to give the bounds extra padding when calling this from the constrained move method.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't see any downsides to adding an extra padding parameter. I have added it here: 6c8b345.

Passing a larger padding parameter does not seem to make the surrounding content more visible when I try it locally, will investigate why the padding does not take effect before requesting re-review

Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds good. I also just double checked the workspace.scrollBoundsIntoView method and realized the default there is 10, so you may also want to set the default here to 10 as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point, I have corrected the default padding to 10 here: 140be72

Re why padding was not taking effect, turns out scrollBoundsIntoView was unexpectedly being called twice in scrollCurrentBlockIntoView. This was because getCurrentBlock eventually calls scrollBoundsIntoView in the call stack. I have suggested a fix for it here: 9657acd

Re adding extra padding for constrained move, I have set the padding as 100 to ensure the block above or below are within view here: 420c224

const blockToView = this.getCurrentBlock(workspace);
if (blockToView) {
workspace.scrollBoundsIntoView(
blockToView.getBoundingRectangleWithoutChildren(),
);
}
}
}

/**
Expand Down
Loading