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

Setting longDescription as absolute #67077

Merged
merged 1 commit into from Jan 28, 2019

Conversation

Projects
None yet
3 participants
@vedipen
Copy link
Member

vedipen commented Jan 24, 2019

No description provided.

@isidorn

This comment has been minimized.

Copy link
Contributor

isidorn commented Jan 25, 2019

I think there is a reason why we want a relative label there
@vedipen why did you make this change? For what part of the UX
@bpasero leaving this one up to you, though I would reject it.

@vedipen

This comment has been minimized.

Copy link
Member Author

vedipen commented Jan 25, 2019

Pointers for change -

  • With relative label, mediumDescription() already does the work
  • Method for longDescription() in untitledEditorInput.ts is not relative under same name
  • In tabsTitleControl.ts method for duplicates, I think this behavior is not expected (for tab labels)

Thank you for looking through.

@bpasero bpasero removed their assignment Jan 28, 2019

@bpasero bpasero added this to the December/January 2019 milestone Jan 28, 2019

@bpasero

This comment has been minimized.

Copy link
Member

bpasero commented Jan 28, 2019

@isidorn this looks like the right change to me and what we currently have is a regression from how it used to be. If you look at https://github.com/Microsoft/vscode/blame/d901821f22e70f233e8ebe16f575647f05f4f537/src/vs/workbench/parts/files/common/editors/fileEditorInput.ts#L151 you can see that the long description is not the relative path. You can tell by looking at the context service not being passed in to the method which is used to produce a relative path.

@isidorn

This comment has been minimized.

Copy link
Contributor

isidorn commented Jan 28, 2019

Fair enough
@vedipen thanks for cathcing this and for submiting this PR. Merging in.

@isidorn isidorn merged commit 89cf3be into Microsoft:master Jan 28, 2019

2 checks passed

VS Code #20190124.66 succeeded
Details
license/cla All CLA requirements met.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment