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 embedding of PDF, video, etc. using image links #351

Merged
merged 4 commits into from
Sep 23, 2022

Conversation

srid
Copy link
Owner

@srid srid commented Sep 23, 2022

Resolves #350

FYI @mnaoumov #341 (comment) - this is how it should be done.

@srid srid marked this pull request as ready for review September 23, 2022 19:26
@srid
Copy link
Owner Author

srid commented Sep 23, 2022

Doing it this way means this syntax will also work for other formats, like video formats:

![](foo.mp4)

@srid srid changed the title Support ![]() to embed PDF Allow embedding of PDF, video, etc. using image links Sep 23, 2022
@srid srid merged commit dc78a19 into master Sep 23, 2022
@srid srid deleted the embed-pdf-as-image branch September 23, 2022 19:32
@mnaoumov
Copy link
Contributor

@srid Maybe Emanote.Pandoc.Link.InlineImage name has to be changed then? Don't you think it is confusing now?

@mnaoumov
Copy link
Contributor

@srid maybe InlineStaticFile would be better ?

@srid
Copy link
Owner Author

srid commented Sep 23, 2022

InlineRef, defined under Emanote.Pandoc, knows nothing about non-Pandoc specific types (like StaticFile). So I'd say it is fine as is. The type exists simply to distinguish between the two possible inline links allowed by Pandoc AST.

@srid
Copy link
Owner Author

srid commented Sep 23, 2022

I actually should refactor the whole pandoc related rendering code one day. It has gotten way too complex. Meanwhile, I decoupled the Heist utilities to https://github.com/srid/heist-extra

@mnaoumov
Copy link
Contributor

@srid I understand that Pandoc mistakenly calls everything Image but I guess your should not keep using that leaking abstraction further in your library. What if you rename it to InlineResource and maybe write in the comment that Pandoc mistakenly calls Text.Pandoc.Definition.Image all resources including images, videos, pdfs etc? This will make your library's API clearer and leaked abstractions from Pandoc will be encapsulated within Emanote.Pandoc.Link module

@srid
Copy link
Owner Author

srid commented Sep 23, 2022

Pandoc mistakenly calls Text.Pandoc.Definition.Image all resources including images, videos, pdfs etc?

Pandoc is behaving correctly. Per commonmark spec, ![]() can only be used for images.

https://commonmark.org/help/tutorial/08-images.html

It just so happens that Obsidian and the like decide to overload this syntax for embedding non-image resources.

If we want to improve the API properly, perhaps we should start from Pandoc itself (as its AST is not that flexible currently); eg.: jgm/pandoc-types#98

@mnaoumov
Copy link
Contributor

@srid ok, I take my words back about misbehaving Pandoc. I'm so used to the embedded videos, audios, pdfs, so I forgot that pure Markdown still doesn't allow it

shivaraj-bh pushed a commit to shivaraj-bh/emanote that referenced this pull request Dec 18, 2023
* Support ![]() to embed PDF

* remove trace

* use embed img syntax

* docs: the same syntax works for videos
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.

Support embedding PDF using regular Markdown syntax
2 participants