feat: scroll moving block into view #451
Conversation
|
For info, we've already merged this to our integration branch for for user testing so no urgency to review. |
RoboErikG
left a comment
RoboErikG
left a comment
There was a problem hiding this comment.
Overall looks really good. Couple comments.
src/actions/mover.ts
Outdated
| * | ||
| * @param workspace The workspace to get current block from. | ||
| */ | ||
| private scrollCurrentBlockIntoView(workspace: WorkspaceSvg) { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
The issue is that scrollBoundsIntoView in scrollCurrentBlockIntoView is not taking effect because it has already been called via getCurrentBlock (which eventually calls scrollBoundsIntoView in the call stack). To bypass getCurrentBlock, a new class variable for current block is set and reset for a move.
src/actions/mover.ts
Outdated
| const blockToView = this.getCurrentBlock(workspace); | ||
| if (blockToView) { | ||
| private scrollCurrentBlockIntoView(workspace: WorkspaceSvg, padding = 10) { | ||
| if (this.currentBlock) { |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I've created RaspberryPiFoundation/blockly#8925 to fix the issue with not scrolling when you increase the padding.
There was a problem hiding this comment.
Sounds good, I have reverted the recent changes which adds a padding for constrained move and avoid multiple calls
RoboErikG
left a comment
RoboErikG
left a comment
There was a problem hiding this comment.
Thanks for working through it with me!
3 participants
Suggested fix for #371
Would welcome an alternative approach to using
setTimeout.Not sure about the expected behaviour, but is perhaps a step forward.