UCSD-PL / vscode

Visual Studio Code
https://code.visualstudio.com
MIT License
0 stars 7 forks source link

Scroll off the end of the world with many PBs #25

Closed michaelbjames closed 1 year ago

michaelbjames commented 1 year ago

Prompt + completion:

import itertools
import matplotlib.pyplot as plt

data = [180,250,130,50]
labels = ["Fall", "Winter", "Spring", "Summer"]

title = "Enrollments by Quarter"

# Generate a bar plot with labels
## ---
plt.bar(labels, data)
plt.title(title)
plt.xlabel("Quarter")
plt.ylabel("Enrollment")
plt.show()

## ---

I get many PBs that extend down off the page. I can scroll down past the end of the editor.

I expected scrolling to move the PBs up so I would be focusing on a PB of a later statement.

Screenshot 2023-02-24 at 9 48 17 AM

rlisahuang commented 1 year ago

can replicate when the scroll occurs within any PB, but not the exploration panel.

rlisahuang commented 1 year ago

This seems to be super tricky to fix. VSCode set the scroll height to be 1000000px, and the boxes are positioned using absolute, with only top and left configured. A possible solution is to configure bottom to be the maximum of 100vh - top and 0 so that at least it sticks to the bottom when 100vh-top is negative.

KasraF commented 1 year ago

It looks like there are two issues here, one is a bug and the other a design decision.

  1. Scrolling off the end of the editor and into the white background: This was a bug and was fixed here.
  2. Based on... something, monaco decides to allow the editor to be scrollable beyond the last line of code. This mostly works, since it allows the user to comfortably keep the code they're working on roughly vertically centered: image

However, we should decide if (and how!) we might want to limit this, and fix the bug where Projection Boxes x position gets too close to the code once we scroll down for some reason: image

@rlisahuang @michaelbjames Does this sound right? Thoughts?

rlisahuang commented 1 year ago

It looks like there are two issues here, one is a bug and the other a design decision.

  1. Scrolling off the end of the editor and into the white background: This was a bug and was fixed here.
  2. Based on... something, monaco decides to allow the editor to be scrollable beyond the last line of code. This mostly works, since it allows the user to comfortably keep the code they're working on roughly vertically centered: image

However, we should decide if (and how!) we might want to limit this, and fix the bug where Projection Boxes x position gets too close to the code once we scroll down for some reason: image

@rlisahuang @michaelbjames Does this sound right? Thoughts?

Solution 1 makes sense and I believe has solved the issue. Re: the monaco design decision... yes, the issue still persists. User can accidentally scroll down too much, and looks like the only way to scroll back is to move the cursor to the right side of the screen (on top of the scroll bar for the editor) then scroll up. I agree that we should put a limit to the maximum scroll for the entire editor to avoid this issue. I think https://github.com/microsoft/vscode/issues/56174 gives us some starting point for what configurations to add to the editor. We can look more into this.

michaelbjames commented 1 year ago

For an MVP, these screenshots, with a grey background look fine. I remember not being able to scroll back up if my mouse were over the (previously) white background. So long as you can scroll back up from anywhere, I think this is a perfectly fine fix!

rlisahuang commented 1 year ago

@KasraF Please close this issue when you push the fix commit

KasraF commented 1 year ago

@KasraF Please close this issue when you push the fix commit

@rlisahuang I'm not sure which commit you're referring to?

I agree with @michaelbjames that scrolling off the editor was the main issue (and 2758248 already fixes that). I played with the scrollBeyondLastLine: false setting and adding padding that https://github.com/microsoft/vscode/issues/56174 discusses, but with that, there's always a chance that there could be a long Projection Box on the last line that the use can't actually scroll down enough to see. And if we add enough padding to make that unlikely, we're back to the current behavior. So I think we can just close this issue.

So unless you disagree, can you please close the issue @rlisahuang?

rlisahuang commented 1 year ago

Oops, my previous comment was attached to the wrong issue. Closing it now.