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 tokuhirom/obsidian-advanced-links-plugin #263

Merged
merged 5 commits into from
May 11, 2021

Conversation

tokuhirom
Copy link
Contributor

[x] I am submitting a new Community Plugin

Repo URL

https://github.com/tokuhirom/obsidian-advanced-links-plugin

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.

@lishid
Copy link
Collaborator

lishid commented Apr 26, 2021

Couple of things in general:

  • I'd suggest updating the plugin name, description and README to explain a bit more what your plugin actually does - it renders 2nd-degree links from the current note at the end of the note. The plugin name "advanced links" and its description was really confusing as to what it does - what does "advanced" mean, and "links to other pages" doesn't explain well the fact that it renders 2nd-degree notes.
  • Consider reformatting your code to use semicolons. I know it's a personal preference but semicolons actually do prevent you from making non-obvious mistakes in javascript. Ref: https://stackoverflow.com/a/1169596

https://github.com/tokuhirom/obsidian-advanced-links-plugin/blob/8ac3ba5f94e90d61c2970ba96d98bd3c85d36c6b/src/main.tsx#L43
activeView can't be MarkdownPreviewView since only MarkdownView contains MarkdownPreviewView.

Actually, you can replace the whole thing with Workspace.getActiveViewOfType(MarkdownView) and perform a null check. Then you can get the file from markdownView.file instead of getting it again from getActiveFile.

https://github.com/tokuhirom/obsidian-advanced-links-plugin/blob/8ac3ba5f94e90d61c2970ba96d98bd3c85d36c6b/src/main.tsx#L62
Don't use .mod-active here, instead, targeting the view.contentEl should be sufficient. Not sure about the OR clause - I don't see the point of it.

https://github.com/tokuhirom/obsidian-advanced-links-plugin/blob/8ac3ba5f94e90d61c2970ba96d98bd3c85d36c6b/src/main.tsx#L90
What does title and path mean here? the parameters required are linktext and sourcePath.

https://github.com/tokuhirom/obsidian-advanced-links-plugin/blob/8ac3ba5f94e90d61c2970ba96d98bd3c85d36c6b/src/main.tsx#L206
Use vault.getAbstractFileByPath(path), then check for instanceof TFile.

https://github.com/tokuhirom/obsidian-advanced-links-plugin/blob/8ac3ba5f94e90d61c2970ba96d98bd3c85d36c6b/src/main.tsx#L209
Wrong null test.

@tokuhirom
Copy link
Contributor Author

At first, thanks for your review.

Couple of things in general:

  • I'd suggest updating the plugin name, description and README to explain a bit more what your plugin actually does - it renders 2nd-degree links from the current note at the end of the note. The plugin name "advanced links" and its description was really confusing as to what it does - what does "advanced" mean, and "links to other pages" doesn't explain well the fact that it renders 2nd-degree notes.

Ah, thanks. I renamed the plugin as obisidian-2hop-links-plugin

  • Consider reformatting your code to use semicolons. I know it's a personal preference but semicolons actually do prevent you from making non-obvious mistakes in javascript. Ref: https://stackoverflow.com/a/1169596

I added prettier to the repo. It added semicolons.

https://github.com/tokuhirom/obsidian-advanced-links-plugin/blob/8ac3ba5f94e90d61c2970ba96d98bd3c85d36c6b/src/main.tsx#L43
activeView can't be MarkdownPreviewView since only MarkdownView contains MarkdownPreviewView.

The basic idea about injecting is taking fromhttps://github.com/mrjackphil/obsidian-backlinks-in-document.
I replaced it with Workspace.getActiveViewOfType.

Actually, you can replace the whole thing with Workspace.getActiveViewOfType(MarkdownView) and perform a null check. Then you can get the file from markdownView.file instead of getting it again from getActiveFile.

Applied!

https://github.com/tokuhirom/obsidian-advanced-links-plugin/blob/8ac3ba5f94e90d61c2970ba96d98bd3c85d36c6b/src/main.tsx#L62
Don't use .mod-active here, instead, targeting the view.contentEl should be sufficient. Not sure about the OR clause - I don't see the point of it.

yep. I replaced it to contentEl.

https://github.com/tokuhirom/obsidian-advanced-links-plugin/blob/8ac3ba5f94e90d61c2970ba96d98bd3c85d36c6b/src/main.tsx#L90
What does title and path mean here? the parameters required are linktext and sourcePath.

I replace them with linkText and sourcePath.

https://github.com/tokuhirom/obsidian-advanced-links-plugin/blob/8ac3ba5f94e90d61c2970ba96d98bd3c85d36c6b/src/main.tsx#L206
Use vault.getAbstractFileByPath(path), then check for instanceof TFile.

Ah, I don't know about the API. I replaced to it.

https://github.com/tokuhirom/obsidian-advanced-links-plugin/blob/8ac3ba5f94e90d61c2970ba96d98bd3c85d36c6b/src/main.tsx#L209
Wrong null test.

Fixed.

@lishid
Copy link
Collaborator

lishid commented May 2, 2021

I replace them with linkText and sourcePath.
https://github.com/tokuhirom/obsidian-2hop-links-plugin/blob/d7a55ca7b4310280c95a6d76fd7396cdf959f60a/src/main.tsx#L261

Seems like you're passing the wrong sourcePath here. It should be activeFile.path (sourcePath means the path of the file that "contains" the link, it means "where to resolve relative paths from")

tokuhirom added a commit to tokuhirom/obsidian-2hop-links-plugin that referenced this pull request May 11, 2021
@tokuhirom
Copy link
Contributor Author

Thanks! I fixed it by this PR: tokuhirom/obsidian-2hop-links-plugin#3

@lishid lishid merged commit 4eda914 into obsidianmd:master May 11, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants