Skip to content

Conversation

@rchiodo
Copy link
Contributor

@rchiodo rchiodo commented May 24, 2022

This PR fixes #145353

Track output size in the notebook and fire off a debounced message whenever the output resizes

In the IW track this event to force scrolling.

[NotebookSetting.interactiveWindowAlwaysScrollOnNewCell]: {
type: 'boolean',
default: true,
markdownDescription: localize('interactiveWindow.alwaysScrollOnNewCell', "Automatically scroll the interactive window to show the output of the last statement executed. If this value is false, the window will only scroll if the last cell was already the one scrolled to.")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Text for this came from the jupyter extension setting it's replacing.

@vscodenpa vscodenpa added this to the May 2022 milestone May 24, 2022
class OutputCell {

public readonly element: HTMLElement;
public readonly bottomElement: HTMLElement;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually I don't need this change. Leftover from another idea I was trying out. I'll revert it.

Copy link
Member

@rebornix rebornix left a comment

Choose a reason for hiding this comment

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

It looks great in overall, only question is if we are duplicating the resize observer, hens not approving yet.

task.start();
task.start(Date.now());
task.executionOrder = 1;
await sleep(10); // Force to be take some time
Copy link
Member

Choose a reason for hiding this comment

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

Why a timeout is required here? I'm afraid a wait/sleep here can introduce potential flakiness.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not required, makes the test behave more like a real world execution.

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 can remove it, the test will still pass, but I thought it made tests more likely to behave like real world execution - so more like testing the real thing.

}, 250);
}

private trackOutputResize(cellId: string, outputContainer: HTMLElement) {
Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No that resize observer is doing something completely different.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

And it's on a different element. It doesn't track the container for all of the output.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It might be repurposed to do this as well as what it was already doing though. I could try that and see if it works too

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looks like that works too, please have another look.

amunger
amunger previously approved these changes May 24, 2022
@rchiodo rchiodo requested a review from rebornix May 24, 2022 20:56
@github-actions github-actions bot locked and limited conversation to collaborators Jul 9, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Output from IW is not scrolled to

4 participants