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

Add optional sortByLabel to QuickPick to control whether to re-sort results #77297

Closed
wants to merge 2 commits into from

Conversation

@pelmers
Copy link
Contributor

pelmers commented Jul 12, 2019

Address issue #73904 by adding an optional sortByLabel to the QuickPick class which determines whether the picker re-sorts the result list when the user types in the input field.

If true, the picker applies a sort to order results by the index of the first appearance of the input in the label.

For backwards compatibility, this field is true by default.

@chrmarti, @roblourens

Notes:
This is just a first pass the implements my initial request -- it doesn't add more fields for highlighting or behaviors that touch fields other than the label.

And I realize that most of the API makes optional params 'false' by default, I'm happy to rename this one to fit that pattern too if requested (e.g. disableSortByLabel: false).

Tested with both 'createQuickPick' and 'showQuickPick' from an extension.

…tems when query changes

Summary:
Address issue #73904 by adding an optional `sortByLabel` to the QuickPick class which determines whether the picker re-sorts the result list when the user types in the input field.

If true, the picker applies a sort to order results by the index of the first appearance of the input in the label.

For backwards compatibility, this field is true by default.

#73904

Test Plan:
attached video shows behavior both before and after

{F167292605}

note: there aren't any existing tests on what happens when the query input changes in the QuickPick

Reviewers: dalongi, ericblue, hchau

Reviewed By: ericblue

Differential Revision: https://phabricator.intern.facebook.com/D16203434

Signature: 16203434:1562878837:5413e3852f2bd04c8e81b9fe5c4a08127dfe3b65
@msftclas

This comment has been minimized.

Copy link

msftclas commented Jul 12, 2019

CLA assistant check
All CLA requirements met.

/**
* An optional flag to sort the final results by index of first query match in label, defaults to true.
*/
sortByLabel?: boolean;

This comment has been minimized.

Copy link
@roblourens

roblourens Jul 15, 2019

Member

These changes should be in vscode.proposed.d.ts, new API starts there until we decide to release it as stable API. More info: https://github.com/Microsoft/vscode/wiki/Extension-API-process

Summary: API additions should start off in the proposed API file.

Test Plan:
same as before, tested that I could skip sorting:
{F168164535}

Reviewers: dalongi, ericblue, hchau

Reviewed By: ericblue

Differential Revision: https://phabricator.intern.facebook.com/D16264910

Signature: 16264910:1563214616:55ecf86a9d4a0d8a0cd44d9b33eb770b2a7d04aa
@pelmers

This comment has been minimized.

Copy link
Contributor Author

pelmers commented Jul 16, 2019

added a region to proposed.d.ts with the changes, I put my name there as well but I'm not sure if I followed the convention correctly

@chrmarti

This comment has been minimized.

Copy link
Contributor

chrmarti commented Nov 18, 2019

Merged manually. Thanks @pelmers !

@chrmarti chrmarti closed this Nov 18, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.