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

Allow inclusion of note when using reference definitions #808

Merged

Conversation

pderaaij
Copy link
Collaborator

Fixes #794.

The idea is that when we want to include a note that is also referred in the link definition, we can just "remove" that definition. This prevents the tokenisation to distort the inclusion format.

@riccardoferretti
Copy link
Collaborator

stupid question: why don't we just remove all (generated) reference definitions from the preview? do they serve a purpose?

@pderaaij
Copy link
Collaborator Author

pderaaij commented Nov 1, 2021

That would lead to the problem for the explicitly defined link references. At this level in the MarkdownIt parser, we can't differentiate between implicit and explicitly generated definitions.

@riccardoferretti
Copy link
Collaborator

IIUC:

  • the goal of including link definitions is so that in preview links can be followed
    • we don't need that for our links (which are source of generated definitions) because we resolve the address in the link itself
    • but the user could have extra definitions, which are needed to resolve the link
  • given that in the preview external (non generated) link definitions are useful, we cannot just remove all link definitions
  • it would be ok to remove all our generated link definitions, but because we can't tell them apart from the external ones, we can't simply do that - instead we can remove the ones that cause us trouble (which we always assume to be internal?):
    • wikilinks with aliases
    • "wikilinks" for included notes

Does it even make sense to have a definition for an embedded note? there is no link to follow there
Maybe the solution here is to not create definitions for included notes?

@pderaaij
Copy link
Collaborator Author

pderaaij commented Nov 1, 2021

* the goal of including link definitions is so that in preview links can be followed

In the context of retaining the definitions for preview, this is correct. In the broader sense, the definitions are there for publication.

  * we don't need that for our links (which are source of generated definitions) because we resolve the address in the link itself
  * but the user could have extra definitions, which are needed to resolve the link

* given that in the preview external (non generated) link definitions are useful, we cannot just remove all link definitions

* it would be ok to remove all our generated link definitions, but because we can't tell them apart from the external ones, we can't simply do that - instead we can remove the ones that cause us trouble (which we always assume to be internal?):
  
  * wikilinks with aliases
  * "wikilinks" for included notes

All above, correct.

Does it even make sense to have a definition for an embedded note? there is no link to follow there Maybe the solution here is to not create definitions for included notes?

We could create some magic to leave it out there. But, that makes life harder for publishing. Not sure what the best UX is here. I could imagine that even embedding does not work when publishing, I at least can manually follow the link because the definition is there.

@riccardoferretti
Copy link
Collaborator

We could create some magic to leave it out there. But, that makes life harder for publishing. Not sure what the best UX is here. I could imagine that even embedding does not work when publishing, I at least can manually follow the link because the definition is there.

Yeah I think we are aligned.
I agree that embedding is outside of our control (it's the publishing fwk's job), but it's fair to keep the link as a sensible fallback.

Ok, let's go forward with your original proposal, will comment in code

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 sounds good to me, I think it would be good to provide better in-code documentation so that our future selves can understand why we went this route

@riccardoferretti
Copy link
Collaborator

Looks great, thx!

@riccardoferretti riccardoferretti merged commit 000da4b into foambubble:master Nov 4, 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.

Link References break Preview for wikilinks
2 participants