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

Use quick pick separator objects instead of a kind #136911

Closed
wants to merge 2 commits into from

Conversation

TylerLeonhardt
Copy link
Member

@connor4312 pointed out that the kind proposal for quickpick separators isn’t ideal because it would not be possible to have a single blank line separating two groups.

and @Tyriar pointed out that kind is ambiguous if you have a situation like this:

kind: ‘a’
kind: ‘b’
kind: ‘a’

we discussed this in the API sync as ‘doc-able’ but I do share the same feeling as he.

As such this implements:

	export interface QuickPickSeparator {
		type: 'separator';
		label?: string;
	}

	export interface QuickPickItem {
		type?: 'item';
	}

This is technically a compile-time break because .items on QuickPick needed to be updated.

Does this PR need to update vscode.d.ts? Is it possible for me to do this totally in vscode.proposed.d.ts?

cc @jrieken

@jrieken
Copy link
Member

jrieken commented Nov 11, 2021

@connor4312 pointed out that the kind proposal for quickpick separators isn’t ideal because it would not be possible to have a single blank line separating two groups.

Are you sure about that? Can't I have some items without kind-property followed by another sequence of items with kind-property? Like [{label: a}, {label: b}, {label: c, kind: k}, {label: d, kind: k}]

and @Tyriar pointed out that kind is ambiguous if you have a situation like this:

I wouldn't say it's ambiguous but at most a programmer error (maybe even wanted). Isn't it the exact same as the following below? I wouldn't say any of the two is better or worse.

[
  { type: 'separator', label: a }, 
  { type: 'item', ... }, 
  { type: 'separator', label: b }, 
  { type: 'item', ... }, 
  { type: 'separator', label: a },
  { type: 'item', ... }
]

This is technically a compile-time break because .items on QuickPick needed to be updated.

Yes, this compile time breakage and also we haven't used type-discriminating properties anywhere. That would be a new concept for the API, which is something we generally try to avoid.

@TylerLeonhardt
Copy link
Member Author

Are you sure about that? Can't I have some items without kind-property followed by another sequence of items with kind-property? Like [{label: a}, {label: b}, {label: c, kind: k}, {label: d, kind: k}]

Yes you can do that but that's not the scenario Connor is calling out. He's calling out:

Asdf
Asdf   
--------------------
Asdf
Asdf

Notice there is no label for this separator. There are no labels for each group. How would that work?

@TylerLeonhardt
Copy link
Member Author

In your example k would be undefined... But then it would be treated as a part of the 1st group which is the problem.

@TylerLeonhardt TylerLeonhardt deleted the TylerLeonhardt/use-separator-objects branch November 12, 2021 19:25
@github-actions github-actions bot locked and limited conversation to collaborators Dec 27, 2021
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.

2 participants