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

Allow scc providers hide commit input box. #60051

Merged
merged 1 commit into from Oct 23, 2018

Conversation

Projects
None yet
3 participants
@IlyaBiryukov

IlyaBiryukov commented Oct 5, 2018

Fix for #51808.

Ilya Biryukov

@joaomoreno joaomoreno added this to the October 2018 milestone Oct 9, 2018

@joaomoreno joaomoreno added the scm label Oct 9, 2018

@joaomoreno joaomoreno merged commit 212aade into Microsoft:master Oct 23, 2018

2 checks passed

VS Code #20181005.64 succeeded with issues
Details
license/cla All CLA requirements met.
Details
@joaomoreno

This comment has been minimized.

Member

joaomoreno commented Oct 23, 2018

Review notes:

  1. There already is a SourceControlInputBox object in the API. This feature should just be a visible property on that object.
  2. The current implementation doesn't actually react on changes to that visible property, it just relies on the value when the SCM repository gets rendered, which is sub-optimal.

I've addressed both these issues and merged this in: f1f5385

Thanks! 🍻


@jrieken Sorry for sidelining this, but this introduces a new proposed API: one which allows to hide a source control provider's input box. Let me know if you want me to bring it up for review in the API call. Here's the signature:

https://github.com/Microsoft/vscode/blob/f1f5385a68bfab396232054a3bb573aa16a05e1d/src/vs/vscode.proposed.d.ts#L729:L738

@jrieken

This comment has been minimized.

Member

jrieken commented Oct 24, 2018

It's OK if it is just proposed but I find the scenario rather weird and LS specific. Would a real source control provider ever want to hide its input field? Should this be linked with the fact that the file system in readonly and then the main side automagically hides/disables the input box?

@jrieken

This comment has been minimized.

Member

jrieken commented Oct 24, 2018

fyi - this has caused #61676 and the merge has been reverted

@joaomoreno

This comment has been minimized.

Member

joaomoreno commented Oct 24, 2018

@jrieken Thanks for jumping on it.

🤦‍♂️ e337569

I am embarrassed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment