-
Notifications
You must be signed in to change notification settings - Fork 209
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
Custom Commands: display commands in sidebar #3245
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Works great! I have one small non-blocking suggested change/improvement.
treeItem.description = item.keybinding | ||
|
||
if (item.key === 'custom' && customCommands?.length) { | ||
treeItem.collapsibleState = vscode.TreeItemCollapsibleState.Expanded |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think given that we're not persisting the collapse/expand state, like the file explorer in VS Code does, it may be safer to always start this as collapsed, in case people in a shared codebase with lots of commands don't want it to always be expanded.
treeItem.collapsibleState = vscode.TreeItemCollapsibleState.Expanded | |
treeItem.collapsibleState = vscode.TreeItemCollapsibleState.Collapsed |
I think ideally we keep it expanded by default if there are commands, but persist/remember if they collapsed it similar to file explorer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If it was collapsed manually by the user, it will stay collapsed until they expanded it again like how it works in the file explorer--is that what you're referring to?
We won't be able to expand it programmatically again once the user has clicked on the collapse button so I think it'd be nice to start it as expanded to let users know about this new field, but happy to change it in a follow up pr if you think we should start the menu as collapsed :) or if we want to start an a/b test for this just lmk! We actually are seeing a lot of people clicking on the New Chat button on the sidebar!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tl;dr the state is persisted I believe
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh! For some reason it wasn’t for me, but if it is, that’s great 👍🏼
PR add custom commands to sidebar as discussed with @toolmantim
Test plan
e2e test for custom commands has been updated accordingly.
custom-sidebar.mov
After
Before