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

Adopt fileResultsNavigation.ts in more places #27458

Closed
bpasero opened this issue May 29, 2017 · 8 comments
Closed

Adopt fileResultsNavigation.ts in more places #27458

bpasero opened this issue May 29, 2017 · 8 comments
Assignees
Labels
debt Code quality issues
Milestone

Comments

@bpasero
Copy link
Member

bpasero commented May 29, 2017

Couple of debt issues around this file in vs/workbench/browser

  • we typically do not talk about files in the core of the workbench (they come in via parts/files). if this is a file specific thing it should move into files namespace, otherwise it should be made generic enough that it can work in any viewer (and maybe moved into some place where we have viewer related utilities?)
  • I am sure we have more places where we open files on click or things in general, should we adopt this everywhere to have a consistent use?
@bpasero bpasero added the debt Code quality issues label May 29, 2017
@bpasero bpasero changed the title FileResultsNavigation is weird fileResultsNavigation.ts is weird May 29, 2017
@sandy081 sandy081 added this to the June 2017 milestone May 31, 2017
@sandy081
Copy link
Member

Agreed to move it under files parts. Regarding adopting it in other places, currently this is being reused in following views.

  • Search
  • Problems
    This gives consistent onFocus and onSelect behaviour while user navigates files in above files. Like, preview the file on focus, open the file to the side on cmd+select.

We can also adopt/reuse the same in other places like

  • Debug
  • File Explorer

@isidorn @bpasero

@bpasero
Copy link
Member Author

bpasero commented Jun 12, 2017

Yeah, makes sense to use it in more places to have consistent picture. Maybe we need to introduce a setting to control if explorer/debug change active file on keyboard navigation. Some people have asked for this.

@bpasero bpasero changed the title fileResultsNavigation.ts is weird Adopt fileResultsNavigation.ts in more places Jun 12, 2017
@isidorn
Copy link
Contributor

isidorn commented Jun 13, 2017

Makes sense to try to reuse this in the debug land

@isidorn isidorn modified the milestones: On Deck, Backlog Jun 13, 2017
@bpasero bpasero removed their assignment Jun 29, 2017
@isidorn
Copy link
Contributor

isidorn commented Nov 13, 2017

@sandy081 I am up for looking into this for November, let me know if you disagree and I can remove the milestone

@isidorn isidorn modified the milestones: On Deck, November 2017 Nov 13, 2017
@sandy081
Copy link
Member

👍

@bpasero bpasero removed the workbench label Nov 16, 2017
@isidorn
Copy link
Contributor

isidorn commented Dec 4, 2017

Moving to December and debt week

@isidorn isidorn modified the milestones: November 2017, December 2017 Dec 4, 2017
@isidorn
Copy link
Contributor

isidorn commented Jan 4, 2018

I have adopted the FileResultsNavigation in the CallStackView.

I could not adopt it in the BreakpointsView since this view uses the List as the underlying implementation and the FileResultsNavigation only supports the Tree. The List and the Tree have different selection events (keyboard not exposed in the list) so it is not trivial to update the FileResultsNavigation to also work with the list.

As for the explorer it is a bit more tricky since it is reacting on selection changes and also on the keyborad / click in the tree controller. I could do a bigger refactoring and try to remove these and only react on selection changes but that will probably break something. Thus I am not very passionate about this. What do you think @bpasero

@isidorn isidorn removed this from the December 2017/January 2018 milestone Jan 4, 2018
@isidorn isidorn added this to the On Deck milestone Jan 4, 2018
@bpasero
Copy link
Member Author

bpasero commented Jan 5, 2018

Yeah looks like there is stuff in the tree (event payload) that would be required in order for the FileResultsNavigation to adopt it for the List...sucks.

@bpasero bpasero closed this as completed Feb 9, 2018
@vscodebot vscodebot bot locked and limited conversation to collaborators Mar 26, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
debt Code quality issues
Projects
None yet
Development

No branches or pull requests

4 participants
@bpasero @isidorn @sandy081 and others