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: terminal suggestion positioning #199420

Merged
merged 3 commits into from Dec 1, 2023

Conversation

cpendery
Copy link
Member

@cpendery cpendery commented Nov 28, 2023

Description

Fixes the position of the terminal suggestions modal.

Testing

Before After
image image
image image

Signed-off-by: Chapman Pendery <cpendery@microsoft.com>
@cpendery cpendery marked this pull request as ready for review November 28, 2023 21:25
if (!dimensions.width || !dimensions.height) {
return;
}
// TODO: What do frozen and auto do?
const xtermBox = this._terminal.element.getBoundingClientRect();
const xtermBox = this._terminal.element!.querySelector('.xterm-screen')!.getBoundingClientRect();
Copy link
Member Author

Choose a reason for hiding this comment

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

Using the .xterm-screen div instead of the terminal element since the suggestions need to account for the gap used to display the command controls.

Without the adjustment:
image
The terminal command controls gap
image

Copy link
Member

Choose a reason for hiding this comment

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

Makes sense!

Copy link
Member

@Tyriar Tyriar left a comment

Choose a reason for hiding this comment

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

Looks great! I'll merge this when we branch the release off (likely this Friday). In the meantime if you want to build more upon this you can checkout with this branch as the base and make the PR against that:

git checkout fix/terminal-suggestion-positioning
git checkout -b fix/terminal-suggestion-positioning__second 

Or just independently if they don't conflict

Comment on lines +270 to +276
private _getTerminalDimensions(): { width: number; height: number } {
return {
width: (this._terminal as any)._core._renderService.dimensions.css.cell.width,
height: (this._terminal as any)._core._renderService.dimensions.css.cell.height,
};
}

Copy link
Member

@Tyriar Tyriar Nov 28, 2023

Choose a reason for hiding this comment

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

Here's a slightly safer way of doing this:

((this._terminal as any)._core as IXtermCore)._renderService.dimensions.css.cell.height

We can also pull _core out on activate so we just do the cast a single time.

if (!dimensions.width || !dimensions.height) {
return;
}
// TODO: What do frozen and auto do?
const xtermBox = this._terminal.element!.getBoundingClientRect();
const xtermBox = this._terminal.element!.querySelector('.xterm-screen')!.getBoundingClientRect();
Copy link
Member

Choose a reason for hiding this comment

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

If/when we move SuggestAddon into a terminalContrib we can do this a single time in ITerminalContribution.xtermOpen

@Tyriar Tyriar added this to the December 2023 milestone Nov 28, 2023
@Tyriar Tyriar merged commit 6506c6e into microsoft:main Dec 1, 2023
6 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Jan 15, 2024
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.

None yet

3 participants