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

enhance main quick pick items filtering logic #852

Merged

Conversation

taras-yemets
Copy link
Contributor

@taras-yemets taras-yemets commented Aug 29, 2023

Next iteration on #631
Follow-up on #798
Design

  1. If input starts with a known command, only the matching item appears in the items list. If a command is unknown, the "no matching commands" item is shown.
  2. Add placeholders to /ask and /fix queries to indicate they expect additional input (question or instruction).
  3. Shows /ask and /fix as fallback items when no items match the query. Query is added to the corresponding quick-pick item label.
Screen.Recording.2023-08-30.at.15.36.03.mov

Test plan

Tested manually (video attached).

@taras-yemets taras-yemets requested a review from a team August 29, 2023 12:34
@taras-yemets taras-yemets marked this pull request as ready for review August 29, 2023 12:34
Copy link
Contributor

@toolmantim toolmantim left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lovely!

Tested locally and works great, except for one case I spotted: once you add arguments, it seems to fall back to the non-matching behaviour:

Screenshot 2023-08-29 at 8 40 06 am

@taras-yemets
Copy link
Contributor Author

Tested locally and works great, except for one case I spotted: once you add arguments, it seems to fall back to the non-matching behaviour:
Screenshot 2023-08-29 at 8 40 06 am

@toolmantim, I think this behavior is expected. It checks whether the query matches either the label ("/ask") or the description ("Ask a question"), but not the combined string in any direction. So it doesn't match queries like "question /ask" or "/ask ask a question".
What do you think about it?
We control how the quick pick items filtering logic so we can tweak the logic described above.

@toolmantim
Copy link
Contributor

@taras-yemets I've spotted another gotcha 😅 /ask doesn't actually support arguments, so showing the fallbacks in this way will be confusing:

Screenshot 2023-08-29 at 2 34 29 pm

In the design, the assumption was that you could provide arguments to both /ask and /fix… but it looks like it's only /fix that does.

I'd still like to get these fallbacks working well though… we're so close. How possible is it to add argument support to /ask as well?

Also i've just updated the Figma to resolve the filtering questions (what to do on no matches, invalid arguments, explicit /command invocation), and some labelling changes to make it clearer which commands support arguments. Unsure which changes to include in this particular PR, but hopefully that resolves the things we've learned/raised so far.

@taras-yemets
Copy link
Contributor Author

In the design, the assumption was that you could provide arguments to both /ask and /fix… but it looks like it's only /fix that does.
I'd still like to get these fallbacks working well though… we're so close. How possible is it to add argument support to /ask as well?

@toolmantim, I think the /ask and /fix fallback options work similarly regarding commands. If a user selects any of them with an empty query, an additional input is shown asking for a question or instruction. If the query is not empty it's used as a /fix command instruction or a question submitted to the sidebar chat.

@taras-yemets taras-yemets changed the title show query in main menu fallback items enhance main quick pick items filtering logic Aug 30, 2023
@taras-yemets
Copy link
Contributor Author

Also i've just updated the Figma to resolve the filtering questions (what to do on no matches, invalid arguments, explicit /command invocation), and some labelling changes to make it clearer which commands support arguments. Unsure which changes to include in this particular PR, but hopefully that resolves the things we've learned/raised so far.

@toolmantim, great work, thank you! Addressed in the current PR (see updated description).

@taras-yemets
Copy link
Contributor Author

@toolmantim, there is a nuance with the UI.
Placeholder (29-31 sec on video) and question/instruction body (9-11 sec on video) are parts of the item's label according to design. Item label is always searchable, and matches are highlighted with a font weight and color - default VSCode's quick-pick behavior.
Unfortunately, we cannot style quick-picks, so we can't change the markup and styles of its items (to do that, we need to reimplement the quick-pick from scratch).
This highlighting seems confusing to me.
Possible solution: render placeholder and question/instruction body as the item description. In this case, we can exclude it from search, but it'll bring style differences compared to the slash command which is rendered as a label. We can do all the content as description in this case, but then it'll differ from other quick-pick items.
What do you think about it?

Copy link
Contributor

@toolmantim toolmantim left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is looking great! 🎉 Nice one.

I gave it a go locally, and the highlighting can feel a bit weird like you said, but given it's pretty common VS Code quickpicks I reckon it's okay:

Screenshot 2023-08-30 at 1 11 02 pm

@toolmantim
Copy link
Contributor

(Don't forget a changelog!)

@taras-yemets taras-yemets merged commit 3821777 into main Aug 31, 2023
9 checks passed
@taras-yemets taras-yemets deleted the taras-yemets/show-query-in-main-quick-pick-fallback-items branch August 31, 2023 08:25
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.

None yet

2 participants