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 support for basic link aliases #223

Closed
wants to merge 2 commits into from
Closed

Conversation

jevakallio
Copy link
Collaborator

@jevakallio jevakallio commented Aug 11, 2020

Note: See Problems below.

Summary

This PR adds basic support for using aliased link syntax [[link|alias]].

Input:

- ✅ Aliases: [[inbox|link alias]]
- ✅ Different aliases to same document: [[inbox|different alias]]
- ✅ Aliases with spaces on either side of the pipe: [[inbox | yet another alias]]
- ✅ Links with no aliases [[inbox]]

Output:

[//begin]: # "Autogenerated link references for markdown compatibility"
[inbox]: inbox "Inbox"
[inbox | yet another alias]: inbox "yet another alias"
[inbox|different alias]: inbox "different alias"
[inbox|link alias]: inbox "link alias"
[//end]: # "Autogenerated link references"

GitHub:
image

Problems

On GitHub pages, piped wikilinks are getting incorrectly parsed to tables 🤦‍♂️

image

But only inside lists:

image

This appears to be an implementation issue in Jekyll and not with GFM. Here's a gist showing the same content rendering fine: https://gist.github.com/jevakallio/0085469e88c9f3e309bda204a1526c0f

Possible solutions

We can add piped alias support, but it won't work on websites using the built-in Jekyll instance (unless we can customise it to not parse the links as tables, so it may not make sense to merge this work until we know how to fix it.

  • Use an alternative aliasing character ([remark-wiki-link default is ":")
  • Override syntax parsing in Jekyll somehow

@jevakallio jevakallio marked this pull request as draft August 11, 2020 23:24
Copy link
Collaborator

@riccardoferretti riccardoferretti left a comment

Choose a reason for hiding this comment

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

The approach looks good.

I feel there is some mingling between link (in a note) and link/connection (in the graph), so I suggested something around that, feel free to take it, comment on it, or leave it.

Should we add a few tests for the various cases?

// - note.links contains all the [[link]] expressions
// - forwardLinks contains all the edges (links) between documents
return (
note.links
Copy link
Collaborator

Choose a reason for hiding this comment

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

shall we make these unique by literalContent? We should probably add a test just in case

e.g.

that was [[a-great-day]] and anytime I have [[a-great-day]] things are good.

(if this turns out to be a bug I guess it was already present in the existing code)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, we should add it here, good catch! Right now the unique check is done on the consumer end after stringifying them, but it makes sense to move over here. Will do!

.map(linkExpression => {
// find the link between this and other document
const link = forwardLinks.find(
note => note.link.slug === linkExpression.slug
Copy link
Collaborator

Choose a reason for hiding this comment

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

I was finding this hard to understand, then I think this parameter is not really note, is it? isn't it the graph connection?

This might partially be a naming issue (as connection/link has a link attribute, which makes things harder to read), not sure what's the best naming pattern we should adopt (maybe connection?)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hmm... yeah, this is confusing to me too now. I'll look into it once I loop back to this PR -- for now I'm not sure if we should move ahead with this until we address the GitHub Pages rendering issues.

);

// if other document is not found, bail
if (!link) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

looking at this I feel that variable should be called connection, as it would probably read more easily and defines a separation between the link in the note and the connection in the graph.

if (!connection) {

(makes me wonder whether the incorrect naming is stemming from the getForwardLinks function, which we might instead wanna call getForwardConnections)

Copy link
Collaborator Author

@jevakallio jevakallio Aug 13, 2020

Choose a reason for hiding this comment

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

Yeah, there definitely is some terminology overloading happening for "link" that we should disambiguate. I like Connection, but also Edge?

In either case, I can name it as per your suggestion as connection here, but I don't think this is the right PR to make that change globally for the entire API.

Copy link
Collaborator

Choose a reason for hiding this comment

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

yeah it's possible that edge is a better name, I suggested connection because that's what we use in NoteGraph, but I am easy about it.
I agree 100% we shouldn't make that refactoring as part of this PR

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