-
-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
Desktop: Expose prompt to plugins as a command #5058
Conversation
comp.setState({ | ||
promptOptions: { | ||
...config, | ||
label: _(config.label), |
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.
Please remove this line - only literal strings can be wrapped in _(). It means the calling code will have to do their own translations.
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, ok. done
answer: answer, | ||
buttonType: buttonType, | ||
}); | ||
comp.setState({ promptOptions: null }); |
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'd put that before resolve
otherwise if the calling code changes something to the UI, for example going to a different screen, you might call setState on an unmounted component.
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.
Got it. Done.
@@ -47,5 +47,6 @@ export default function() { | |||
'toggleSafeMode', | |||
'showShareNoteDialog', | |||
'showShareFolderDialog', | |||
'showPrompt', |
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 the name says, these commands are those that show up in the menu, but this one shouldn't so you can remove this.
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.
Right, done.
|
||
export const runtime = (comp: any): CommandRuntime => { | ||
return { | ||
execute: async (_context: CommandContext, config: any) => { |
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.
Would you mind defining an interface for the config? It's not too hard to do and it would make the command more usable. It would start like this:
interface PromptConfig {
label: string;
description?: string;
// etc.
}
If you search for promptOptions:
in the codebase you'll find the possible properties.
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.
Sure, done.
This PR is ready for another review. |
|
||
interface PromptConfig { | ||
label: string; | ||
inputType?: 'dropdown' | 'datetime' | 'tags' | 'text'; |
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.
Your code is correct, but TypeScript has various issues with fields defined as string options like this. So instead it's better to create an enum type that has all the possible values.
So inputType?: PromptInputType
And then:
enum PromptInputType {
Dropdown = 'dropdown',
// etc.
}
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.
Ah okay. Done.
enum PromptInputType { | ||
Dropdown = 'dropdown', | ||
Datetime = 'datetime', | ||
Tags = 'tags', | ||
Text = 'text', | ||
} |
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.
Use tabs. Could you make sure you have the commit hook installed as it's quite annoying having to ask for something like this. Or maybe you made changes directly on GitHub without testing which is even worse.
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.
Done. That's weird, I don't know why this happened. Usually, the hook runs every time for me. I guess I committed while being in packages/app-desktop
directory. Maybe that's why the hooks didn't run.
This PR is ready for another review. |
Looks good now, thanks for the update. |
Adds a command to show the prompt and get the user input so that it can be used by plugins.