-
Notifications
You must be signed in to change notification settings - Fork 263
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.
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
Use CSS scroll anchoring for most chat scroll pinning #704
Conversation
374d129
to
dfeccfb
Compare
@dominiccooney I verified all the 5 steps and they work perfectly for me. I do have a followup question about step 4. ❤️ Great work ❤️ |
Agree that the search results appear too fast. There's a few opportunities to make this better; see #714. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice. This feels much better!
At a glance the animation seemed a bit slow, and I wondered if we should support reduced-motion
, but I see it's using the native scrollTo
properties, so that's okay speed wise. I assume under the hood it supports reduced motion at the O/S preference level?
The only gotcha I found is that it now scrolls back to the bottom whenever you switch back to the Cody pane from another pane.
e.g. If you have a long chat and scroll up to copy+paste some code, switch to File Explorer, open a file, then re-open the Cody pane… it scrolls down and your code goes out of view. We should retain the scroll position.
Although looking at the code, that could be unrelated to this PR, but now just more obvious and annoying because of the smooth scroll?
|
||
.scroll-anchor { | ||
overflow-anchor: auto; | ||
height: 5px; /* this is the grippy region for autoscroll */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was worried this was too small, but after testing, any larger and it would fight with your ability to scroll 👍🏼
@toolmantim good catch, I think that is a regression in this PR. Will look. |
dfeccfb
to
fd5d01e
Compare
I've updated this so it doesn't scroll on switch away and back. In the edge case that you switch away, then LLM response streams in, then switch back we keep the scroll position so you can read from there. Chromium turns off scroll animations for prefers-reduced-motion. I'm assuming there's some way to set that in vscode. |
Chat scroll pinning was broken for
/search
because that adds messages in quick succession and the React effect was only looking at the change in message author.The general approach was also iffy because we were basically polling the scroll position on updates, but in practice scrolling works best when it is managed asynchronously by the browser.
This takes a different approach:
I've made some subtle UX changes like animating some of the scrolls.
The scroll anchor adds a 5px gutter at the bottom of the content which is the "grippy" region for the scroll. We can avoid that extra space if we want, I took a look at it, it requires a couple of stacking contexts and I thought it wasn't worth it. But we can do it if we want.
Test plan
/search foo
and the scroll position should keep up with the bottom of the content. (This comes in very fast and I think we should add a heuristic to maybe not do that. But it is better than the behavior today, which quietly drops the new content below the fold.)