Skip to content
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

Merged
merged 11 commits into from
Nov 28, 2023

Conversation

akoreman
Copy link
Contributor

@akoreman akoreman commented Nov 22, 2023

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.

Copy link

codecov bot commented Nov 22, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (ee917cf) 87.54% compared to head (1c7a39b) 87.54%.

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     
Flag Coverage Δ
unittests 87.54% <100.00%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@akoreman akoreman marked this pull request as ready for review November 23, 2023 09:48
@akoreman akoreman changed the title fix: improvements for completer popup + inline preview fix: try to scroll inline preview into view Nov 24, 2023
this.popupTimer.schedule();
this.tooltipTimer.schedule();
} else {
this.$updatePopupPosition();
Copy link
Contributor

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?

Copy link
Contributor Author

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.delayedCalls which can be scheduled to happen after a timeout to allow some time for other stuff to happen.

Copy link
Contributor

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.

Copy link
Contributor Author

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.

Copy link
Contributor Author

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

Copy link
Contributor

@andredcoliveira andredcoliveira left a 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.

src/virtual_renderer.js Outdated Show resolved Hide resolved
src/virtual_renderer.js Outdated Show resolved Hide resolved
@akoreman akoreman merged commit 95af560 into ajaxorg:master Nov 28, 2023
3 checks passed
@akoreman akoreman deleted the inline_scroll branch November 28, 2023 12:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants