Skip to content

Conversation

@yoshum
Copy link

@yoshum yoshum commented Feb 23, 2023

Thank you for the excellent extension! I like Markdown Notes so much that I recommended it to my colleagues, but I also want to see this improved a bit.

Summary

This PR addresses #6 and makes wiki-links work in Markdown preview.

What's changed

  • Supply href with paths relative to the workspace root
  • Set data-href as well as href for A-tags

The first change was necessary because

  1. VSCode's Uri.path starts with "/" and represents a full path in the file system, but
  2. href starting with "/" is interpreted as a path relative to the document root, which in this case is the workspace root.

Therefore, to get the desired behavior, we need to fill href with a path relative to the document or the workspace folder.

Regarding the second change, it seems necessary for links in the preview window to work as expected.

Implementation details

Regarding the second change, I chose to copy, paste and modify the code from markdown-it-wikilinks, which is a markdown-it plugin that renders wiki-links. A cleaner approach would be to use a version of markdown-it-wikilinks that sets the data-href attribute (e.g. @shdwcat/markdown-it-wikilinks, see also thomaskoppelaar/markdown-it-wikilinks#1). I made this choice because I just wanted to have it work as quickly as possible without worrying about any side effects caused by changing dependent packages.

Limitations

A rendered wiki-link would be incorrect if "vscodeMarkdownNotes.workspaceFilenameConvention": "relativePaths" or the wiki-link itself is a relative path.


// Transformation that only gets applied to the page name (ex: the "test-file.md" part of [[test-file.md | Description goes here]]).
export function postProcessPageName(pageName: string) {
function postProcessPageName(pageName: string) {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are these function name changes just cleanup to match the names in pluginSettings?

}

// Transformation that only gets applied to the link label (ex: the " Description goes here" part of [[test-file.md | Description goes here]])
export function postProcessLabel(label: string) {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are these export removed?

postProcessPageName: postProcessPageName,
postProcessLabel: postProcessLabel,
uriSuffix: `.${NoteWorkspace.defaultFileExtension()}`,
description_then_file: NoteWorkspace.pipedWikiLinksSyntax() == 'desc|file',
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will the new version work with the piped syntax desc|file?

"watch": "tsc -watch -p ./"
},
"dependencies": {
"@thomaskoppelaar/markdown-it-wikilinks": "^1.3.0",
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Trying to understand other implications of this change. Your goal is to make it so the wiki links are clickable inside the vscode markdown preview -- but does this break any other use cases? Eg, is this going to break things somehow for html generated for other purposes / for viewing outside of the Markdown Prview?

@kortina
Copy link
Owner

kortina commented Mar 5, 2023

Also, is it possible to add tests for any of this stuff?

@kortina
Copy link
Owner

kortina commented Mar 5, 2023

How much more work would it be to take the "cleaner approach" re "cleaner approach would be to use a version of markdown-it-wikilinks that sets the data-href attribute" ?

@yoshum
Copy link
Author

yoshum commented Mar 18, 2023

Also, is it possible to add tests for any of this stuff?

I'd be happy to, but because I am not familiar with testing a markdown-it plugin, it will take some time before it's done.

How much more work would it be to take the "cleaner approach"

Probably the hardest part is to make sure that the version of markdown-it-wikilinks like @shdwcat/markdown-it-wikilinks does not break any of the current features. Once that's done, then adopting it is as easy as rolling back all the changes in this PR and switching from @thomaskoppelaar/markdown-it-wikilinks to e.g. @shdwcat/markdown-it-wikilinks.

The current PR, instead, makes a minimal change to the code in @thomaskoppelaar/markdown-it-wikilinks to avoid side effects.

@kortina
Copy link
Owner

kortina commented Mar 18, 2023

I see. OK, well, I leave it to you to decide which approach to take, but either way it would be good to add tests to this. And maybe put a few screenshots / a screencast in the PR to demonstrate what this looks like / how it works. 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.

2 participants