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: Interactive Code Blocks #3654

Merged
merged 4 commits into from
Jun 21, 2024
Merged

Conversation

ernstbolt
Copy link

I am submitting a new Community Plugin

Repo URL

Link to my plugin:

Release Checklist

  • I have tested the plugin on
    • Windows
    • macOS
    • [x ] Linux
    • Android (if applicable)
    • iOS (if applicable)
  • 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 developer policies at https://docs.obsidian.md/Developer+policies, and have assessed my plugins's adherence to these policies.
  • [x ] I have read the tips in https://docs.obsidian.md/Plugins/Releasing/Plugin+guidelines 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.

@github-actions github-actions bot changed the title Add interactive code block plugin Add plugin: Windesheim's Interactive Code Blocks Jun 5, 2024
@ObsidianReviewBot
Copy link
Collaborator

Thank you for your submission, an automated scan of your plugin code's revealed the following issues:

Required

[1][2]:Using innerHTML, outerHTML or similar API's is a security risk. Instead, use the DOM API or the Obsidian helper functions: https://docs.obsidian.md/Plugins/User+interface/HTML+elements


Do NOT open a new PR for re-validation.
Once you have pushed all of the required changes to your repo, the bot will update the labels on this PR within 6 hours.
If you think some of the required changes are incorrect, please comment with /skip and the reason why you think the results are incorrect.

@ObsidianReviewBot ObsidianReviewBot self-assigned this Jun 6, 2024
@ObsidianReviewBot ObsidianReviewBot added Changes requested Additional review required PR needs to be reviewed by another person, after the currently requested changes have been made Ready for review and removed Ready for review Changes requested Additional review required PR needs to be reviewed by another person, after the currently requested changes have been made labels Jun 6, 2024
@ObsidianReviewBot ObsidianReviewBot removed their assignment Jun 6, 2024
@ObsidianReviewBot
Copy link
Collaborator

Changes requested by bot have been made, assigning human for additional review.

@joethei
Copy link
Collaborator

joethei commented Jun 6, 2024

"name": "Windesheim's Interactive Code Blocks",, "description": "Preview Windeheim's interactive code block in obsidian!",
I don't see a reason to include the name "Windesheim" in the name & description.
Also please don't include "Obsidian" in the description.

script.src = "https://cdn.jsdelivr.net/gh/windesheim-hbo-ict/deeltaken@latest/CodeBlock/codeBlockLite.js"
This should not be loaded externally, but bundled into the plugin, including all of it's dependencies.
Also, that external dependency is setting innerHTML in a lot of places, without the need to.
A bunch of strings are also in dutch, while the plugin readme is in english.

Also, it's not documented that your plugin only works in reading mode, please add that.

@joethei joethei added Changes requested Minor changes requested PR can be merged after some final changes have been requested and removed Ready for review labels Jun 6, 2024
@ObsidianReviewBot ObsidianReviewBot added Ready for review Changes made and removed Changes requested Minor changes requested PR can be merged after some final changes have been requested labels Jun 6, 2024
Copy link

github-actions bot commented Jun 6, 2024

Hello!

I found the following issues in your plugin submission

Errors:

❌ Plugin name mismatch, the name in this PR (Windesheim's Interactive Code Blocks) is not the same as the one in your repo (Interactive Code Blocks). If you just changed your plugin name, remember to change it in the manifest.json in your repo and your latest GitHub release.


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.

@ernstbolt ernstbolt changed the title Add plugin: Windesheim's Interactive Code Blocks Add plugin: Interactive Code Blocks Jun 7, 2024
@ernstbolt ernstbolt closed this Jun 7, 2024
@ernstbolt ernstbolt reopened this Jun 7, 2024
@ItsSiem
Copy link
Contributor

ItsSiem commented Jun 7, 2024

@joethei I have fixed all of the things you said in your review. Sorry for the mention but the Validation bot does not seem to want to review the PR.

@ernstbolt ernstbolt closed this Jun 13, 2024
@joethei joethei reopened this Jun 13, 2024
@joethei
Copy link
Collaborator

joethei commented Jun 13, 2024

<style>
This should be in the styles.css file, not loaded via innerHTML.


And this should also not be using innerHTML.
Please see our guidelines: https://docs.obsidian.md/Plugins/Releasing/Plugin+guidelines#Avoid+%60innerHTML%60%2C+%60outerHTML%60+and+%60insertAdjacentHTML%60

await fetch('http://localhost:8080/code', {
Use the requestUrl function from the Obsidian API instead, it will handle some things like CORS automatically.

@joethei joethei added Changes requested Minor changes requested PR can be merged after some final changes have been requested and removed Validation failed Changes made labels Jun 13, 2024
@ItsSiem
Copy link
Contributor

ItsSiem commented Jun 13, 2024

I've removed the last innerHTML uses. The styles can not be moved to the styles.css file because they are used within the shadowDOM of the webcomponent.

@ObsidianReviewBot ObsidianReviewBot added Ready for review Changes made and removed Changes requested Minor changes requested PR can be merged after some final changes have been requested labels Jun 14, 2024
@joethei
Copy link
Collaborator

joethei commented Jun 19, 2024

console.log(output);
Please avoid unnecessary logging. If you need it for debugging purposes, add a check to only log during development.

@joethei joethei added Changes requested Minor changes requested PR can be merged after some final changes have been requested and removed Ready for review Changes made labels Jun 19, 2024
@ItsSiem
Copy link
Contributor

ItsSiem commented Jun 21, 2024

I have removed the console.log

@ObsidianReviewBot ObsidianReviewBot added Ready for review Changes made and removed Changes requested Minor changes requested PR can be merged after some final changes have been requested labels Jun 21, 2024
@joethei joethei merged commit d50477b into obsidianmd:master Jun 21, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants