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

promote local media assets to file attachments #250

Merged
merged 5 commits into from
Nov 29, 2023
Merged

Conversation

cinxmo
Copy link
Contributor

@cinxmo cinxmo commented Nov 28, 2023

closes #54

@Fil
Copy link
Contributor

Fil commented Nov 28, 2023

Nice! In #54 (comment) I started a list of elements we should support.

@cinxmo cinxmo force-pushed the cindy/file-attachments branch from 006b229 to 848bb4e Compare November 28, 2023 21:16
@cinxmo cinxmo changed the title [WIP] promote local media assets to file attachments promote local media assets to file attachments Nov 28, 2023
@cinxmo cinxmo marked this pull request as ready for review November 28, 2023 21:57
src/markdown.ts Outdated
{query: "video source[src]", src: "src"},
{query: "audio[src]", src: "src"},
{query: "audio source[src]", src: "src"},
{query: "link[href]", src: "href"}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I purposely didn't add a[href] because we already link to local .md files. See test case:
https://github.com/observablehq/cli/blob/67ebf20b3c1b993f9fc664fd8cd827353d371b65/test/input/build/config/index.md?plain=1#L3-L4

Copy link
Contributor

Choose a reason for hiding this comment

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

The question of a[href] highlights a small flaw in our logic.

Here are three cases:

  • [another page](page) — the page is served from page.md, it's not a file attachment.
  • [view source](file.md) — file.md should be a file attachment
  • <a href="earthquakes.csv" download>download dataset</a> — data.csv should be a file attachment

Currently we base our choice on the tag: in the current approach audio, video, link… are never pointing to pages, and thus indicate a file attachment, and conversely a always indicates a page and never a file attachment.

But shouldn't this be associated to the URL routing instead? If the URL is routed to a md source (or to a static asset #169), it's not a file attachment; otherwise it's a (potential) file attachment.

That new logic could be applied to all the tags. Of course in practice an audio[src] will never be a page (since all pages generate HTML, which are not suitable for audio); but a link[href] can be a page — e.g., <link rel="next"> [ref].

Copy link
Contributor

Choose a reason for hiding this comment

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

(Note that this issue will be solved with #257.)

src/markdown.ts Show resolved Hide resolved
@cinxmo cinxmo requested a review from Fil November 28, 2023 22:08
src/markdown.ts Outdated Show resolved Hide resolved
@Fil Fil enabled auto-merge (squash) November 29, 2023 10:07
@Fil Fil merged commit d3f1475 into main Nov 29, 2023
1 check passed
@Fil Fil deleted the cindy/file-attachments branch November 29, 2023 10:08
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.

Promote img, video, audio, and picture elements to file attachments
2 participants