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

Skip explorer reveal on visible items #90626

Merged
merged 3 commits into from
Feb 14, 2020
Merged

Skip explorer reveal on visible items #90626

merged 3 commits into from
Feb 14, 2020

Conversation

baileyherbert
Copy link
Contributor

Fixes #82828.

Before this change, clicking on an editor tab while explorer.autoReveal is true would always scroll the explorer so that the corresponding file was vertically centered.

After this change, the explorer isn't scrolled if the corresponding file is already visible in the file tree.

@isidorn
Copy link
Contributor

isidorn commented Feb 14, 2020

I think changing this behavior makes sense overall. However I do not like the approach that there are a lot of changes in the tree land.
The tree and list should not be aware of this. I believe you can just use tree api from the explorer to get this behavior.
fyi @joaomoreno

@joaomoreno joaomoreno self-requested a review February 14, 2020 09:34
Copy link
Member

@joaomoreno joaomoreno left a comment

Choose a reason for hiding this comment

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

This already exists in the list/tree:

/**
* Returns the relative position of an element rendered in the list.
* Returns `null` if the element isn't *entirely* in the visible viewport.
*/
getRelativeTop(index: number): number | null {
if (index < 0 || index >= this.length) {

So we don't need the list/tree changes.

@baileyherbert
Copy link
Contributor Author

Thanks guys. Let's try again. 😊

@isidorn
Copy link
Contributor

isidorn commented Feb 14, 2020

Works great. Love it.
Even when the user triggers "Reveal active file in sidebar" we center it no matter the visibility which is correct behavior since this is explicitly requirested by user.
Thanks a lot. Merging in.

@isidorn isidorn merged commit 536e816 into microsoft:master Feb 14, 2020
@isidorn isidorn added this to the February 2020 milestone Feb 14, 2020
@baileyherbert baileyherbert deleted the 82828-listview-reveal branch February 14, 2020 19:27
@github-actions github-actions bot locked and limited conversation to collaborators Mar 30, 2020
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.

autoReveal highlight filename where it is without always moving it to the middle in explorer
3 participants