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

Created generic quick pick util class to support various use cases #1272

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

gnana997
Copy link
Contributor

Summary of Changes

  • Added a generic createQuickPick utility class to handle multiple scenarios exposing a callback functions for required usecase.

Any additional details or context that should be provided?

Pull request checklist

Please check if your PR fulfills the following (if applicable):

Tests
  • Added new
  • Updated existing
  • Deleted existing
Other
  • All new disposables (event listeners, views, channels, etc.) collected as for eventual cleanup?
  • Does anything in this PR need to be mentioned in the user-facing CHANGELOG or README?
  • Have you validated this change locally by packaging and installing the extension .vsix file?
    gulp clicktest

Signed-off-by: gnana997 <gnana097@gmail.com>
@shouples shouples self-assigned this Mar 18, 2025
Signed-off-by: gnana997 <gnana097@gmail.com>
@gnana997
Copy link
Contributor Author

Hi @shouples , I went through the localResource.ts and I have updated the utils quick pick to customize the createQuickPick in localResource.ts. Can you check it once and let me know if I am missing something?
Thanks!

@shouples
Copy link
Contributor

shouples commented Mar 21, 2025

Hey @gnana997, this is working nicely so far! Just one test failure running locally (via gulp test) on MacOS 15.3 (arm64):

  862 passing (4s)
  2 pending
  1 failing
  1) QuickPick utils
       should create a QuickPick with correct options:
     AssertionError [ERR_ASSERTION]: Expected values to be loosely deep-equal:

[
  {
    label: 'Item 2'
  }
]

should loosely deep-equal

{
  label: 'Item 2'
}

My only suggestion for now would be to include QuickPickItemWithValue as the primary item type (instead of QuickPickItem) in quickPickUtils, and to provide some examples in the PR description or JSDoc comments for how to use the function(s) for simple quickpick use-cases as well as more complicated/customized use-cases.

Thanks for helping out with this -- I'll try to provide a more thorough review next week.

@gnana997 gnana997 force-pushed the create-quick-pick branch from b2b37d5 to 272cac8 Compare March 22, 2025 13:32
@gnana997 gnana997 marked this pull request as ready for review March 22, 2025 13:32
@gnana997 gnana997 requested a review from a team as a code owner March 22, 2025 13:32
@gnana997
Copy link
Contributor Author

Hi @shouples , I have updated the failing test case and also updated the class to use QuickPickItemWithValue along with JSDoc comments on usage.

Should I start migrating functions with showQuickPick to use showEnhancedQuickPick created?

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants