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

[Feature Request] YouTube Embeds #896

Open
Stanzilla opened this issue Apr 15, 2020 · 16 comments
Open

[Feature Request] YouTube Embeds #896

Stanzilla opened this issue Apr 15, 2020 · 16 comments
Labels
C: magiclink Related to the magiclink extension. P: maybe Pending approval of low priority request. T: feature Feature.

Comments

@Stanzilla
Copy link
Contributor

I currently do some very ugly hacks in JavaScript at the moment to get a few missing Markdown features working.

One of them is a tag for YouTube embeds. Currently I'm scanning the alt tag of images for YOUTUBE and then replace the image with an iframe like so:

const elements = document.querySelectorAll("img[alt=\"YOUTUBE\"]")

  Array.prototype.forEach.call(elements, (el, i) => {
    const id = el.getAttribute("title").split("/")[el.getAttribute("title").split("/").length - 1]
    const oldClass = el.getAttribute("class")
    const iframe = document.createElement("iframe")

    iframe.title = "YouTube"
    iframe.src = `https://www.youtube.com/embed/${id}?modestbranding=1&`

    if (oldClass !== "null") {
      iframe.className = `video-embed ${oldClass}`
    } else {
      iframe.className = "video-embed"
    }

    el.replaceWith(iframe)
  })

Now if we could do this natively and on compile instead of run, that would be great.

@gir-bot gir-bot added the S: triage Issue needs triage. label Apr 15, 2020
@facelessuser
Copy link
Owner

@gir-bot add S: duplicate
@gir-bot remove S: triage

Duplicate of #897

@gir-bot gir-bot added S: duplicate The issue has been previously reported. and removed S: triage Issue needs triage. labels Apr 15, 2020
@Stanzilla
Copy link
Contributor Author

Sort of disagree with this being a dupe, the implementation would be quite different, native video tag vs iframe

@facelessuser
Copy link
Owner

I'm looking at this as a discussion to embed media. This is a different source of media I guess.

So it sounds like you want a youtube specifc plugin? Or do you want it to handle other video services too? There is a lot of magic happening here in this image link.

And it feels weird doing it like this. Why not just provide a youtube link and have it translated to the iframe. Much like MagicLink does with Github links etc? Why with image tags?

@facelessuser facelessuser reopened this Apr 15, 2020
@facelessuser
Copy link
Owner

I'm not yet sure if I want to maintain a youtube extension yet, but I want to understand this approach as it seems awkward.

@Stanzilla
Copy link
Contributor Author

The thing is, youtube embeds only work with iframes, so yes, in theory it could work with every other video service that requires this but we'd then have to have the url as parameter or a different tag per service.

Why not just provide a youtube link and have it translated to the iframe. Much like MagicLink does with Github links etc?

That's what I want, yes

Why with image tags?

Was the easiest because I could abuse the alt tag

@facelessuser
Copy link
Owner

I can consider this, it would be more a task for MagicLink. I can't promise I'll take it on, but it is a reasonable request.

@gir-bot remove S: duplicate
@gir-bot add S: Feature, C: magiclink

@gir-bot gir-bot added C: magiclink Related to the magiclink extension. and removed S: duplicate The issue has been previously reported. labels Apr 15, 2020
@facelessuser
Copy link
Owner

Whoops!

@gir-bot add T: feature

@gir-bot gir-bot added the T: feature Feature. label Apr 15, 2020
@kdeldycke
Copy link

kdeldycke commented Sep 2, 2020

Maybe this feature is already covered by mdx-video? In which case we can try to take over maintainership and refresh it.

@facelessuser
Copy link
Owner

Going through the backlog again. Not sure I'd bother taking ownership of mdx-video I'd probably just extend MagicLink. If the link followed a certain pattern, we'd generically wrap the link in such a way that it would work.

@facelessuser facelessuser added the P: maybe Pending approval of low priority request. label Sep 22, 2021
@facelessuser
Copy link
Owner

facelessuser commented Sep 23, 2021

I think with #897 on its way, and thinking about this more, I think I kind of see how I may approach this. I imagine we will just look for link patterns in the tree. We might abstract it so people can write their own functions for more niche video services, but the basic idea is that we'll crawl the tree looking for anchor tags, and loop through our supported services seeing:

  1. That the href matches the pattern for the given service.
  2. the anchor link has no content. If you are trying to display link text and other HTML content under it, we'll probably skip it. We may make an exception if href == content as that is how magiclink autolinks raw URLs. We could just throw away such content, but my initial thinking is to just ignore anchors of this sort.

After that, we just wrap it up in an iframe. Users can add whatever iframe attributes they want via attr_list, but maybe we'll allow, specifying globally some attributes, maybe even per service. Like allowfullscreen etc.

I kind of want it general so I don't have to manually keep a list of every service and support lesser-known niche services. So abstracting it in such a way as to allow some flexibility will probably be key. In the end, I don't even think it needs to rely on MagicLink specifically. I'm thinking that both #897 and this one can probably be moved into some media type extension.

@facelessuser
Copy link
Owner

I have something basic working for this as well. It'll just find the links and convert them. I'm only supporting Youtube, Vimeo, and Dailymotion out of the box. People can extend it if they like. I will probably only support very popular reputable sites officially.

Some sites in the mdx_video plugin weren't worth supporting. Metacafe redirects to something different these days, etc. This will all get bundled in whatever I end up calling the new media embedding plugin.

@kdeldycke
Copy link

Basic support of the main 3 video platforms you mentioned, associated with the generic support mechanism is perfect to show any user how to tweak and extend the configuration to their needs.

Thanks a lot @facelessuser for these sensible choices! Indeed, I don't want you to burnout trying to maintain the whole list of videos platform out there.

@facelessuser
Copy link
Owner

I will probably end up using the ![](youtubelink) syntax for media service embedding as well. There is no concept of alt on an <iframe>, but generally, you should always specify a title on an <iframe>. We'll probably just use the alt as the title. If a user happens to define a title in some other way, that will get used instead.

This way, embedding video will be an explicit action, not implicit. And you can just link a video externally if you like without embedding it as well.

![My title](youtubelink)

Let me know if this sounds crazy, but I think I like this a bit better. And it keeps both embedding physical media and media services consistent with the option of linking to videos without embedding if desired.

@facelessuser
Copy link
Owner

Additionally, if no alt or title is provided, I believe we'll add something generic like YouTube video player as a title, etc.

@facelessuser
Copy link
Owner

#1465 Experimental branch now has video service embedding and normal audio/video embedding. Still needs tests, but this so far is the final proposal unless feedback steers it in a different direction. I also squeezed in subtitle support in normal video/audio support via <track> elements using .vtt sources.

@facelessuser
Copy link
Owner

#1777 may be a better way to implement this

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C: magiclink Related to the magiclink extension. P: maybe Pending approval of low priority request. T: feature Feature.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants