macOS: Jump palette #9970
macOS: Jump palette #9970
Conversation
The main differences here are: - The only commands are the focus commands - Different UI messages and not prefixing with "focus: "
mitchellh
left a comment
mitchellh
left a comment
There was a problem hiding this comment.
Thanks! I've submitted some stylistic reviews, overall architecture looks right.
| /// result in the view disappearing. | ||
| @Binding var isPresented: Bool | ||
|
|
||
| /// Set this to true for the jump palette mode (only focus action). |
There was a problem hiding this comment.
I'd just change this to an enum of mode (two values). And it doesn't have to be a @Binding since its only one-way.
| if !isJumpPalette { | ||
| // Updates always appear first | ||
| options.append(contentsOf: updateOptions) | ||
| } |
There was a problem hiding this comment.
Change this to a switch on mode
|
|
||
| return CommandOption( | ||
| title: "Focus: \(displayTitle)", | ||
| title: isJumpPalette ? displayTitle : "Focus: \(displayTitle)", |
There was a problem hiding this comment.
Remove this completely and change it so that in command palette mode we run a map over it which modifies the title to add a prefix.
|
|
||
| /// The command palette is reused for the focus palette, this indicates what | ||
| /// mode it should be in, true for the focus palette. | ||
| var commandPaletteIsJumpPalette: Bool { get set } |
There was a problem hiding this comment.
Replace commandPaletteIsShowing with commandPaletteState and CommandPaletteState is an enum with 3 values: hidden, command, jump.
|
|
||
| struct CommandPaletteView: View { | ||
| @Binding var isPresented: Bool | ||
| @Binding var isJumpPalette: Bool |
There was a problem hiding this comment.
Remove this, doesn't have to be a binding, and just make it placeholder: String and let downstream callers set it.
|
Thanks a lot for such a quick review, all of your comments make sense to me, I haven't really written Swift before (and am not particularly trying to learn), so I'm happy if you want to just pick up the change and clean it up the way you prefer. If not I can try to address the comments, but the back and forth might just be more work in the end. Thanks! |
|
Edited the title to note that this PR is specific to macOS. |
3 participants
Hey @mitchellh
This is a rough attempt at adding the "jump palette" mode for only changing between windows.
A few notes:
Thanks a ton, now that I'm using this it's so great!