Skip to content

feat: scroll moving block into view #451

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.

Sign up for GitHub

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

Merged
merged 8 commits into from
Apr 25, 2025
Merged

feat: scroll moving block into view #451

Changes from 5 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
726bdfe
feat: scroll moving block into view
microbit-grace Apr 17, 2025
2bc7d7c
chore: tweak doc and format mover.ts
microbit-grace Apr 22, 2025
6c8b345
feat: add padding param (def to 0) for mover scroll into view
microbit-grace Apr 22, 2025
140be72
fix: correct default padding to 10 for move scroll to view
microbit-grace Apr 23, 2025
9657acd
fix: bypass get cur block when move scroll into view
microbit-grace Apr 23, 2025
420c224
feat: set constrained move block spacing to 100
microbit-grace Apr 23, 2025
9019851
Revert "feat: set constrained move block spacing to 100"
microbit-grace Apr 23, 2025
f78d28e
Revert "fix: bypass get cur block when move scroll into view"
microbit-grace Apr 23, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
41 changes: 30 additions & 11 deletions src/actions/mover.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,12 @@ import {Navigation} from '../navigation';
*/
const UNCONSTRAINED_MOVE_DISTANCE = 20;

/**
* Amount of space to put between a moving block and the edge of
* the workspace's viewport, when making a constrained move.
*/
const CONSTRAINED_MOVE_BLOCK_SPACING = 100;

/**
* Low-level code for moving blocks with keyboard shortcuts.
*/
Expand Down Expand Up @@ -55,6 +61,12 @@ export class Mover {
*/
private oldDragStrategy: IDragStrategy | null = null;

/**
* The current block being moved, which is set at the start of a move and
* reset at the end of a move.
*/
private currentBlock: BlockSvg | null = null;

constructor(protected navigation: Navigation) {}

/**
Expand Down Expand Up @@ -101,6 +113,7 @@ export class Mover {
const cursor = workspace?.getCursor();
const block = this.getCurrentBlock(workspace);
if (!cursor || !block) throw new Error('precondition failure');
this.currentBlock = block;

// Select and focus block.
common.setSelected(block);
Expand Down Expand Up @@ -141,11 +154,13 @@ export class Mover {
new utils.Coordinate(0, 0),
);

// Scroll into view after block has finished moving.
setTimeout(() => this.scrollCurrentBlockIntoView(workspace), 0);

this.unpatchWorkspace(workspace);
this.unpatchDragStrategy(info.block);
this.moves.delete(workspace);
// Delay scroll until after block has finished moving.
setTimeout(() => this.scrollCurrentBlockIntoView(workspace), 0)
this.currentBlock = null;
return true;
}

Expand Down Expand Up @@ -174,11 +189,13 @@ export class Mover {
new utils.Coordinate(0, 0),
);

// Scroll into view after block has finished moving.
setTimeout(() => this.scrollCurrentBlockIntoView(workspace), 0);

this.unpatchWorkspace(workspace);
this.unpatchDragStrategy(info.block);
this.moves.delete(workspace);
// Delay scroll until after block has finished moving.
setTimeout(() => this.scrollCurrentBlockIntoView(workspace), 0)
this.currentBlock = null;
return true;
}

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

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

Expand Down Expand Up @@ -315,15 +332,17 @@ export class Mover {
}

/**
* Scrolls the current block into view if exists one.
* Scrolls the current block into view if one exists.
*
* @param workspace The workspace to get current block from.
* @param workspace The workspace to scroll the given bounds into view in.
* @param padding Amount of spacing to put between the bounds and the edge of
* the workspace's viewport.
*/
private scrollCurrentBlockIntoView(workspace: WorkspaceSvg) {
const blockToView = this.getCurrentBlock(workspace);
if (blockToView) {
private scrollCurrentBlockIntoView(workspace: WorkspaceSvg, padding = 10) {
if (this.currentBlock) {
Copy link
Contributor

@RoboErikG RoboErikG Apr 23, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I think this will always be null when called from setTimeout since you set this.currentBlock to null immediately after setting a timeout to call this method, so the assignment to null happens before the timeout function is called.

Your previous version where you got the current block in this method should have worked. What was the reason for storing it in a property?

Copy link
Contributor

@RoboErikG RoboErikG Apr 23, 2025

Choose a reason for hiding this comment

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

Ah, I'm seeing it now. Since it won't scroll if the block is already visible regardless of the extra padding the second call with a larger padding gets ignored, but this is equivalent to just removing all of the calls with setTimeout.

For now, can you revert the change to avoid multiple calls and we can merge it without the extra padding working? I think this will need a change to scrollBoundsIntoView to optionally require padding to be included.

Copy link
Contributor

@RoboErikG RoboErikG Apr 23, 2025

Choose a reason for hiding this comment

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

I've created RaspberryPiFoundation/blockly#8925 to fix the issue with not scrolling when you increase the padding.

Copy link
Contributor Author

@microbit-grace microbit-grace Apr 24, 2025

Choose a reason for hiding this comment

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

Sounds good, I have reverted the recent changes which adds a padding for constrained move and avoid multiple calls

workspace.scrollBoundsIntoView(
blockToView.getBoundingRectangleWithoutChildren(),
this.currentBlock.getBoundingRectangleWithoutChildren(),
padding,
);
}
}
Expand Down