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 plugin: obsidian-ivre-plugin #1490

Merged
merged 1 commit into from
Jan 24, 2023
Merged

Conversation

p-l-
Copy link
Contributor

@p-l- p-l- commented Dec 31, 2022

I am submitting a new Community Plugin

Repo URL

Link to my plugin: https://github.com/ivre/obsidian-ivre-plugin

Release Checklist

  • I have tested the plugin on
    • Windows
    • macOS
    • Linux
    • Android (if applicable)
    • iOS (if applicable)
  • My GitHub release contains all required files
    • main.js
    • manifest.json
    • styles.css (optional)
  • GitHub release name matches the exact version number specified in my 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.
  • My README.md describes the plugin's purpose and provides clear usage instructions.
  • I have read the tips in https://github.com/obsidianmd/obsidian-releases/blob/master/plugin-review.md and have self-reviewed my plugin to avoid these common pitfalls.
  • I have added a license in the LICENSE file.
  • My project respects and is compatible with the original license of any code from other plugins that I'm using.
    I have given proper attribution to these other projects in my README.md.

@p-l- p-l- force-pushed the add-ivre-plugin branch 3 times, most recently from 10dcc68 to 87a173d Compare January 3, 2023 08:49
@p-l- p-l- force-pushed the add-ivre-plugin branch 6 times, most recently from 2696c3b to 7749ddb Compare January 15, 2023 16:03
@p-l- p-l- force-pushed the add-ivre-plugin branch 2 times, most recently from bf8a417 to 89c1e71 Compare January 18, 2023 19:22
@liamcain
Copy link
Collaborator

  • this.app this is not defined. It might be working but it's falling back to the global scope, which I don't think you should be relying on. It's currently accessible from the global context for debugging purposes, and might be removed in the future. Instead, you should use the reference to App that you have from your plugin instance and just pass that into your functions.
  • this.app same thing
  • this.app here too
  • this.app.vault and here
  • this.app. hmm, okay there's a lot of them. I won't point them all out
  • getActiveFile this can return null. Also, since you're passing in a MarkdownView to this function, you should be comparing it to the view.file and not the active file
  • ivre you don't need to prefix the ID with your plugin name, it will get automatically prefixed with it by the addCommand

@liamcain liamcain self-assigned this Jan 24, 2023
p-l- added a commit to p-l-/obsidian-ivre-plugin that referenced this pull request Jan 24, 2023
Based on comment in obsidianmd/obsidian-releases#1490:

- Remove out-of-scope `this.app`
- Validate `getActiveFile()` return value
- Remove the "ivre-" prefix from the command name
@github-actions
Copy link

Hello!

I found the following issues in your plugin submission

Warnings:

⚠️ There's no need to include obsidian in the plugin ID. The ID is used for your plugin's settings folder so it should closely match your plugin name for user convenience.


This check was done automatically.

@github-actions
Copy link

Hello!

I found the following issues in your plugin submission

Errors:

❌ Plugin ID mismatch, the ID in this PR is not the same as the one in your repo.


Warnings:

⚠️ There's no need to include obsidian in the plugin ID. The ID is used for your plugin's settings folder so it should closely match your plugin name for user convenience.


This check was done automatically.

@github-actions
Copy link

Hello!

I found the following issues in your plugin submission

Warnings:

⚠️ There's no need to include obsidian in the plugin ID. The ID is used for your plugin's settings folder so it should closely match your plugin name for user convenience.


This check was done automatically.

@p-l-
Copy link
Contributor Author

p-l- commented Jan 24, 2023

@liamcain thanks a lot for your review! Sorry for all the mistakes, I'm a total noob in both TypeScript and the Obsidian API!

Hope it's better now, let me know if there are other things to fix. Release 1.2.0 contains your proposed fixes.

@liamcain
Copy link
Collaborator

p-l- added a commit to p-l-/obsidian-ivre-plugin that referenced this pull request Jan 24, 2023
Based on comment in obsidianmd/obsidian-releases#1490:

- Remove this.app.workspace.getActiveFile calls
@p-l-
Copy link
Contributor Author

p-l- commented Jan 24, 2023

Thanks again @liamcain! That's done. Do you see something else that should be done?

@liamcain liamcain merged commit deea54d into obsidianmd:master Jan 24, 2023
@liamcain
Copy link
Collaborator

LGTM, merged!

@p-l- p-l- deleted the add-ivre-plugin branch January 24, 2023 16:56
@p-l-
Copy link
Contributor Author

p-l- commented Jan 24, 2023

Thanks!

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.

3 participants