-
Notifications
You must be signed in to change notification settings - Fork 5.3k
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
fix: try to scroll inline preview into view #5400
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #5400 +/- ##
=======================================
Coverage 87.54% 87.54%
=======================================
Files 583 583
Lines 46222 46253 +31
Branches 7001 7003 +2
=======================================
+ Hits 40465 40493 +28
- Misses 5757 5760 +3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
src/autocomplete.js
Outdated
this.popupTimer.schedule(); | ||
this.tooltipTimer.schedule(); | ||
} else { | ||
this.$updatePopupPosition(); |
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.
What does the following code do?
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.
this.tooltipTimer
updates the position of the doc tooltip, this.popupTimer
updates the position of the popup itself. They are both lang.delayedCall
s which can be scheduled to happen after a timeout to allow some time for other stuff to happen.
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.
Why does it happen outside the this.inlineRenderer && this.inlineEnabled
case? Is it to cancel if inline rendering gets disabled? Or is it a general improvement for the non-inline case? If so, we should update the GH description.
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.
this.popupTimer
calls this.$updatePopupPosition
so the same thing happens in both flows, only that for the inline case we wait 50ms to account for the scrolling which potentially happens.
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.
tried to make this a bit more clear in a new commit
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.
Looks good. Left a couple of small comments.
Issue #, if available: NA
Description of changes: Currently, when inline preview is used at the bottom of the file, it will be invisible. This changes it to scroll down enough to get the inline preview into view. If the inline preview is bigger than the view, we scroll down so that the cursor is at the top of the screen to get the most possible into view
Screen.Recording.2023-11-23.at.14.34.06.mov
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.