obsidian-releases icon indicating copy to clipboard operation
obsidian-releases copied to clipboard

updating community-plugins with meeting notes plugin

Open TimHi opened this issue 2 years ago • 4 comments

I am submitting a new Community Plugin

Repo URL

Link to my plugin: https://github.com/TimHi/obsidian-meeting-notes

Release Checklist

  • [ ] I have tested the plugin on
    • [ ] Windows
    • [x] macOS
    • [ ] Linux
    • [ ] Android (if applicable)
    • [ ] iOS (if applicable)
  • [x] My GitHub release contains all required files
    • [x] main.js
    • [x] manifest.json
    • [ ] styles.css (optional)
  • [x] 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)
  • [x] The id in my manifest.json matches the id in the community-plugins.json file.
  • [x] My README.md describes the plugin's purpose and provides clear usage instructions.
  • [x] 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.
  • [x] I have added a license in the LICENSE file.
  • [x] 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.

TimHi avatar Jul 12 '22 09:07 TimHi

await this.loadSettings(); You can just read from the settings object, as you are always updating it on settings change. I also don't see you accessing the settings object anywhre in this function, so I don't think this is even necessary here.

You are using semicolons most of the time, but are missing them sometimes, not having semicolons can lead to rare, hard to debug issues: https://flaviocopes.com/javascript-automatic-semicolon-insertion/(this is not an issue in this plugin, but letting you know for future reference).

joethei avatar Jul 15 '22 07:07 joethei

@joethei Added semicolons and removed the settings loading, tested without and it worked fine recognizing folder changes in the settings. Bumped the version to 1.0.2 and added a new release.

TimHi avatar Jul 16 '22 07:07 TimHi

  • https://github.com/TimHi/obsidian-meeting-notes/blob/763bdc3692fcc06ab0e11f498d9ca5179ad53914/src/filehandler/FileService.ts#L44 Should should probably also make sure that the file extension is .md
  • https://github.com/TimHi/obsidian-meeting-notes/blob/763bdc3692fcc06ab0e11f498d9ca5179ad53914/src/filehandler/FileRenderer.ts#L7 Obsidian uses {{date}} for all of its core templating plugins (Daily Notes, Unique Note Creator, Templates), so it probably makes sense to stay consistent with that in case users want to use the same note as a template for the templates plugin down the line.
  • https://github.com/TimHi/obsidian-meeting-notes/blob/763bdc3692fcc06ab0e11f498d9ca5179ad53914/src/filehandler/FileRenderer.ts#L3 I don't really see much reason for this to be a class, it could just be a function.

liamcain avatar Jul 21 '22 18:07 liamcain

@liamcain Thanks for the suggestions, I added the check for the markdown extension and exchanged {Date} with {{Date}} as templating the generated file is something I plan to do as soon as I get the time.

When doing the template I plan to use the FileRenderer class for it so it will be used, thats just my preference to keep it seperated.

TimHi avatar Jul 21 '22 19:07 TimHi