-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
Add launchMode parameter to ToggleCommandPalette action #8382
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.
Great work! @zadjii-msft will probably want to take a look at this. But this is a weird week because of Thanksgiving, so it may be a bit before some other team members get to it.
I'll approve after some discussion on the CommandPaletteMode
thing below.
src/cascadia/LocalTests_SettingsModel/KeyBindingsTests.cpp-9d33445a
Outdated
Show resolved
Hide resolved
_switchToMode(CommandPaletteMode::ActionMode); | ||
auto mode = (launchMode == CommandPaletteLaunchMode::CommandLine) ? | ||
CommandPaletteMode::CommandlineMode : | ||
CommandPaletteMode::ActionMode; |
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.
Could/Should we consolidate these into one CommandPaletteMode
enum in ActionArgs.idl? That way we don't have to do this translation? Or is that a weird thing to do since the mode that the command palette is in isn't really a setting. (this one is more of a thought exercise than a "requested change")
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.
Thought exercise already completed in #8322 (comment) and #8322 (comment)
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.
As Dustin mentioned, we didn't want to expose the entire enum, so I thought that the introduction of the new one was the simplest way to achieve this.
Thanks for the review! Will get to the fixes soon. |
af52b8a
to
4466a26
Compare
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.
Love this. Thanks so much.
Only nits, but since you have to rev the PR for code formatting reasons, I'll still sign off 😄
(obligated to wait for docs pr, but this time we're only at one approval) |
Added documentation (though something very basic, but I believe it is better this way). |
@zadjii-msft - if you are here, please take a look 😊 |
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.
Yep, this looks good to me. Thanks!
Hello @DHowett! Because this pull request has the p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (
|
@DHowett - it seems that automerge fails |
Oop lemme kick that build. Sorry! |
Gods of CI please be kind with this one.. don't let it fail randomly.. |
@ msftbot re-run the CI till it passes I kid, we really should do something about these CI failures |
@zadjii-msft - it worked! I guess it is a combination of my prayers and the undocumented msftbot APIs 😆 |
🎉 Handy links: |
PR Checklist
Detailed Description of the Pull Request / Additional comments
Added an optional launchMode parameter to "commandPalette" command.
The values of the launchMode are either "action" (default) or "command line".
Validation Steps Performed