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

Command: remove slashes from commands #3061

Merged
merged 19 commits into from
Feb 13, 2024
Merged

Command: remove slashes from commands #3061

merged 19 commits into from
Feb 13, 2024

Conversation

abeatrix
Copy link
Contributor

@abeatrix abeatrix commented Feb 6, 2024

CLOSE #1730

Remove slashes from command names and implemented design from https://www.figma.com/file/jEsnzgsf0hNuJbqB9pKaGm/VS-Code---Cody---Commands-up-front-and-ChatGPT%2B%2B?type=design&node-id=1550-62492&mode=design&t=qdjbLWsl9qHq578p-4

Main Commands Menu

image

Custom Commands Menu

image

image

image

Test plan

Updated all current tests.

Screen.Recording.2024-02-09.at.7.49.47.AM.mov

Before

image

image

@abeatrix abeatrix requested review from toolmantim and a team February 6, 2024 23:59
vscode/src/commands/CommandsController.ts Outdated Show resolved Hide resolved
vscode/src/commands/menus/index.ts Outdated Show resolved Hide resolved
@toolmantim
Copy link
Contributor

Design wise we want to have it match the commands treeview.

Could we update both the treeview and the command menu to match this?
https://www.figma.com/file/jEsnzgsf0hNuJbqB9pKaGm/VS-Code---Cody---Commands-up-front-and-ChatGPT%2B%2B?type=design&node-id=1550-62492&mode=design&t=qdjbLWsl9qHq578p-4

@abeatrix
Copy link
Contributor Author

abeatrix commented Feb 7, 2024

Design wise we want to have it match the commands treeview.

Could we update both the treeview and the command menu to match this?

https://www.figma.com/file/jEsnzgsf0hNuJbqB9pKaGm/VS-Code---Cody---Commands-up-front-and-ChatGPT%2B%2B?type=design&node-id=1550-62492&mode=design&t=qdjbLWsl9qHq578p-4

@toolmantim sure can do! Forgot we have a figma for this so this would make things a lot easier, thanks!

@abeatrix
Copy link
Contributor Author

abeatrix commented Feb 9, 2024

@toolmantim updated and ready for review!

@abeatrix abeatrix changed the title update: remove slashes from commands Command: remove slashes from commands Feb 10, 2024
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 great!

A few small changes if we could:

  • "New Chat" from the quickpick should act the same as "New Chat" in the review (open a new chat tab)
  • Could we add the keyboard shortcuts for "New Chat" and "Edit Code" to the quickpick item descriptions?
  • If there are no custom commands, can we make "Custom Commands" quickpick item & treeview item go directly to the "Configure Custom Commands…" and skip the empty-ish middle quickpick?

One bug:

  • In the quickpick "Custom Commands" > "Configure Custom Commands…" both the "New Custom Command..." and "Open Custom Commands Documentation" items don't seem to work

@abeatrix
Copy link
Contributor Author

@toolmantim updated based on your suggestions and fixed the bug:

  • "New Chat" from the quickpick should act the same as "New Chat" in the review (open a new chat tab)
  • Could we add the keyboard shortcuts for "New Chat" and "Edit Code" to the quickpick item descriptions?
    • Also added a keyboard shortcuts for "Custom Commands" as shown in your figma design
  • If there are no custom commands, can we make "Custom Commands" quickpick item & treeview item go directly to the "Configure Custom Commands…" and skip the empty-ish middle quickpick?
  • Fixed bug: In the quickpick "Custom Commands" > "Configure Custom Commands…" both the "New Custom Command..." and "Open Custom Commands Documentation" items don't seem to work

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.

Nice! Two non-blocking things…

If you start typing "New" you end up seeing duplicates. I'd suggest we always exclude the "New Chat (shortcut)" and "Edit Code (shortcut)" items when you start filtering/typing… because they're always going to be there as "Start a new chat" and "Start a new edit" anyhow.

CleanShot 2024-02-13 at 16 18 16@2x

Also when you filter in custom commands we probably don't want to show these last 3 items, just the filtered custom commands (or nothing if no matches):

CleanShot 2024-02-13 at 16 18 40@2x

@toolmantim
Copy link
Contributor

Does this binding actually exist? I can't get it to trigger:

CleanShot 2024-02-13 at 16 24 19@2x

If it doesn't exist, we can just remove that shortcut label for now.

@abeatrix
Copy link
Contributor Author

Does this binding actually exist? I can't get it to trigger:

CleanShot 2024-02-13 at 16 24 19@2x

If it doesn't exist, we can just remove that shortcut label for now.

it does exist! I just fixed this and the bugs you found. Will update the e2e tests tomorrow to cover these cases before I finalize the PR tomorrow, thanks for taking the time to review and test this PR out!

Copy link
Member

@valerybugakov valerybugakov left a comment

Choose a reason for hiding this comment

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

Looks great!

vscode/src/commands/index.ts Outdated Show resolved Hide resolved
Comment on lines +24 to 27
* cody command, e.g. '/ask'
* @deprecated Use 'commandKey' instead.
*/
slashCommand?: string
Copy link
Member

Choose a reason for hiding this comment

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

Nice! 🙌

@abeatrix abeatrix merged commit 8e6278a into main Feb 13, 2024
13 of 14 checks passed
@abeatrix abeatrix deleted the bee/remove-slashes branch February 13, 2024 17:21
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.

Remove Slash from Slash Command
3 participants