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

feat: add obsidian-code-files #1805

Merged
merged 10 commits into from
Apr 20, 2023
Merged

feat: add obsidian-code-files #1805

merged 10 commits into from
Apr 20, 2023

Conversation

lukasbach
Copy link
Contributor

I am submitting a new Community Plugin

Repo URL

Link to my plugin: https://github.com/lukasbach/obsidian-code-files

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.

@github-actions
Copy link

Hello!

I found the following issues in your plugin submission

Errors:

❌ You modified files other than community-plugins.json.
❌ It seems like you made a typo in the repository field lukasbach/obsidian-code-files.
❌ Please don't use the word obsidian in the plugin ID. The ID is used for your plugin's folder so keeping it short and simple avoids clutter and helps with sorting.
❌ Please don't use the word plugin in the plugin ID. The ID is used for your plugin's folder so keeping it short and simple avoids clutter and helps with sorting.
❌ You don't have a manifest.json at the root of your repo, or it could not be parsed.


Warnings:

⚠️ Your repository does not include a license. It is generally recommended for open-source projects to have a license. Go to https://choosealicense.com/ to compare different open source licenses.


This check was done automatically. Do NOT open a new PR for re-validation. Instead, to trigger this check again, make a change to your PR and wait a few minutes, or close and re-open it.

@github-actions
Copy link

Hello!

I found the following issues in your plugin submission

Errors:

❌ You modified files other than community-plugins.json.


This check was done automatically. Do NOT open a new PR for re-validation. Instead, to trigger this check again, make a change to your PR and wait a few minutes, or close and re-open it.

@joethei
Copy link
Collaborator

joethei commented Mar 27, 2023

id: 'code-files-create',
Obsidian will automatically prefix the command id with the plugin id,
you don't need to include it yourself.

this.iframe.src = https://embeddable-monaco.lukasbach.com?${queryParameters.toString()};
This plugin should not require an internet connection to function.

@lukasbach
Copy link
Contributor Author

lukasbach commented Mar 27, 2023

Thanks for reviewing the plugin!

This plugin should not require an internet connection to function.

Yeah I figured that, but unfortunately I really didn't get Monaco to work directly with Obsidian. Monaco has a very strict setup with web workers, which to my knowledge requires multiple files that are available under customizable URLs and which the website hosting the editor needs to load during runtime, and I really didn't find a way to get that to work with Obsidians restriction that the entire plugin code needs to be one single file.

If you have alternative suggestions I would be happy to give them a try, but otherwise, I would love if we could add the plugin with this online-dependency as a caveat. I can also add this info to the top of the readme to make this caveat more visible to people interested in adding the plugin.

@joethei
Copy link
Collaborator

joethei commented Mar 28, 2023

You can run a web worker inside of Obsidian, you will need a esbuild plugin that can bundle it inside of the main.js.
I know that a few plugins do that, but I don't recall their names.

If you can't get that to work, please absolutely mention it, prominently mention that it connects to your server in the README.

@lukasbach
Copy link
Contributor Author

Hi @joethei. Something like that would probably be the solution, but I'm still running into issues because of Monaco's expectations of how the worker files are resolved, and the way Monaco expects the loader framework to be augmented.

I might give it another try in the future, but would prefer to leave it as is for now. I documented the caveat in the Readme, and also fixed your suggestion on the command id prefix above.

@lukasbach
Copy link
Contributor Author

Hi @joethei, I don't want to pressure and understand that you have other important things to do, but am wondering if there is something I can do to support getting this merged?

@liamcain
Copy link
Collaborator

  • this.containerEl.getElementsByClassName("view-content")[0] this can be this.contentEl then you don't need to query the dom for it.
  • existingFile you also need to check that existingFile is instanceof TFile (since it can return a TFolder too)
  • obsidian Capitalize Obsidian please 😄. Also, you can just check if Obsidian is in dark mode instead of asking users to toggle the switch. Just check if app.getTheme() === 'obsidian'

@liamcain liamcain merged commit f6de616 into obsidianmd:master Apr 20, 2023
@lukasbach
Copy link
Contributor Author

Hey, thanks for taking your time to review, I've fixed the things you've noted. One side-note, the method app.getTheme() does not seem to be included in the official obsidian typings, I think that might be helpful to many others as well if it is visible in the types 🙂

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