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

Don't require Esc to dismiss Cody menu #700

Merged
merged 5 commits into from
Aug 16, 2023

Conversation

toolmantim
Copy link
Contributor

This removes the ignoreFocusOut: true settings on our command quickpick, removing the need for you use to press esc in the input to dismiss it. This makes it feel just as quick and light as the standard cmd-shift-p command palette.

The one downside to removing it is if you need to change your code selection mid-quickpick-item-selection, but given the standard command palette has commands that depend on code selection too (e.g. "Transform to Kebab Case") I think we should follow their suit and not introduce the extra bit of friction that requiring esc adds.

Test plan

  • Opened command palette
  • Clicked outside it and verified it dismisses

@@ -83,7 +83,6 @@ export class CustomCommandsBuilderMenu {
title: 'Select the context to include with the prompt for the new command',
placeHolder: 'Tip: Providing limited but precise context helps Cody provide more relevant answers',
canPickMany: true,
ignoreFocusOut: false,
Copy link
Contributor

Choose a reason for hiding this comment

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

let's keep this one? For users who are following the tutorial steps in the notebook might want to switch between screens during the new command building step (happened to me plenty of time 😅 ).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, I removed it because ignoreFocusOut: false is the default value and thought it was a noop. Want me to re-add, or change it to ignoreFocusOut: true?

Copy link
Contributor

Choose a reason for hiding this comment

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

by "let's keep this one?" i meant to say set it to true ignoreFocusOut: true,*

Copy link
Contributor

Choose a reason for hiding this comment

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

github is slow for me today 😅 Yea let's change it to ignoreFocusOut: true?

Copy link
Contributor

Choose a reason for hiding this comment

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

(Thanks!)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure why it was false to begin with, but I think it makes sense to be true like you said. I've just updated it.

@@ -83,7 +83,6 @@ export class CustomCommandsBuilderMenu {
title: 'Select the context to include with the prompt for the new command',
placeHolder: 'Tip: Providing limited but precise context helps Cody provide more relevant answers',
canPickMany: true,
ignoreFocusOut: false,
Copy link
Contributor

Choose a reason for hiding this comment

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

by "let's keep this one?" i meant to say set it to true ignoreFocusOut: true,*

@@ -83,7 +83,6 @@ export class CustomCommandsBuilderMenu {
title: 'Select the context to include with the prompt for the new command',
placeHolder: 'Tip: Providing limited but precise context helps Cody provide more relevant answers',
canPickMany: true,
ignoreFocusOut: false,
Copy link
Contributor

Choose a reason for hiding this comment

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

github is slow for me today 😅 Yea let's change it to ignoreFocusOut: true?

@@ -83,7 +83,6 @@ export class CustomCommandsBuilderMenu {
title: 'Select the context to include with the prompt for the new command',
placeHolder: 'Tip: Providing limited but precise context helps Cody provide more relevant answers',
canPickMany: true,
ignoreFocusOut: false,
Copy link
Contributor

Choose a reason for hiding this comment

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

(Thanks!)

@toolmantim toolmantim merged commit 1d2b595 into main Aug 16, 2023
8 checks passed
@toolmantim toolmantim deleted the tl/cody-menu-should-disappear-on-focusout branch August 16, 2023 05:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants