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

Add Pluck plugin #303

Closed
wants to merge 1 commit into from
Closed

Add Pluck plugin #303

wants to merge 1 commit into from

Conversation

kevboh
Copy link
Contributor

@kevboh kevboh commented May 21, 2021

[x] I am submitting a new Community Plugin

Repo URL

https://github.com/kevboh/obsidian-pluck

Release Checklist

  • I have tested this on Windows, macOS, and Linux (if applicable)
  • Github release contains all required files
    • main.js
    • manifest.json
    • styles.css (optional)
  • Github release name matches the exact version number specified in your manifest.json (Note: Use the exact version number, don't include a prefix v)
  • The id in my manifest.json matches the id in the community-plugins.json file.
  • README clearly describes the plugins purpose and provides clear usage instructions.

@Vinzent03
Copy link
Contributor

Why are you using editor.getDoc()? Isn't editor already what you need?
https://github.com/kevboh/obsidian-pluck/blob/master/main.ts#L68

@kevboh
Copy link
Contributor Author

kevboh commented May 21, 2021

Why are you using editor.getDoc()? Isn't editor already what you need?

https://github.com/kevboh/obsidian-pluck/blob/master/main.ts#L68

I copied this pattern from Templater; happy to use editor directly if that's the recommended approach.

@lishid
Copy link
Collaborator

lishid commented May 24, 2021

https://github.com/kevboh/obsidian-pluck/blob/4879e8484ed891b6b01f084102ffe9eef130a73d/main.ts#L18

Don't do this. This would modify the behavior of everything inside Obsidian, including making the use of <iframe>s insecure. Instead, you can look into using electron's net.request (https://www.electronjs.org/docs/api/net) (which uses chromium's connection mechanism, including using the OS proxy settings) or node-fetch (which uses node's http/https libs underneath, ignoring proxy settings).

https://github.com/kevboh/obsidian-pluck/blob/4879e8484ed891b6b01f084102ffe9eef130a73d/main.ts#L61
You should probably test this condition first before attempting to download the file.

https://github.com/kevboh/obsidian-pluck/blob/4879e8484ed891b6b01f084102ffe9eef130a73d/main.ts#L68
getDoc is unnecessary as it's just getDoc() { return this }.

https://github.com/kevboh/obsidian-pluck/blob/4879e8484ed891b6b01f084102ffe9eef130a73d/main.ts#L73
Seems like you just need a regular Modal with a textbox? FuzzySuggestModal is definitely not what you're looking for.

@kevboh
Copy link
Contributor Author

kevboh commented May 26, 2021

Instead, you can look into using electron's net.request (https://www.electronjs.org/docs/api/net) (which uses chromium's connection mechanism, including using the OS proxy settings) or node-fetch (which uses node's http/https libs underneath, ignoring proxy settings).

Oddly, I tried node-fetch first, and it was also hitting CORS errors. It's possible I misconfigured it.

You should probably test this condition first before attempting to download the file.

Great idea, thanks.

getDoc is unnecessary as it's just getDoc() { return this }.

Good to know. As I said above, I copied that code straight from Templater.

Seems like you just need a regular Modal with a textbox? FuzzySuggestModal is definitely not what you're looking for.

Fair enough. I really like the UI of FuzzySuggestModal, it feels faster and more vs-code like to me. But I can switch it over.

Thanks for the review!

@kevboh kevboh closed this Jun 17, 2021
@kevboh kevboh mentioned this pull request Jun 19, 2021
7 tasks
@kevboh
Copy link
Contributor Author

kevboh commented Jun 19, 2021

I've updated this plugin to address the issues raised here and opened a new PR for the new version.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants