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

Implement a web component to handle <oembed url=""> #2737

Open
Reinmar opened this issue Aug 23, 2018 · 13 comments
Open

Implement a web component to handle <oembed url=""> #2737

Reinmar opened this issue Aug 23, 2018 · 13 comments
Labels
package:media-embed status:discussion type:docs This issue reports a task related to documentation (e.g. an idea for a guide).

Comments

@Reinmar
Copy link
Member

Reinmar commented Aug 23, 2018

In the semantic mode (config.media.semanticDataOutput) the editor produces <oembed> tags. Maybe we can include a simple WC in the docs for handling those with Embedly/Iframely? Or is it pointless because Embedly/Iframely's JS libs allow replacing these elements automatically?

@iparamonau
Copy link

Hi @Reinmar, Ivan from Iframely here. I see you guys redoing the media embeds integration. We took a look and see great work all around. The focus on semantic output for actual articles is appreciated.

So, a comment simply about the documentation to save us all from unnecessary support requests.

Take our +1 to expand and state it more prominently that it is recommended to convert <oembed>s for published articles on the server:

  • First, it might enhance incorrect aspect-ratios for the hardcoded YouTube, Vimeo et al players. Individual videos vary in those ratios. You'll get quite a few nudges about that. It does require at least some server work to retrieve the proper ones.
  • Then, performance. Say, Iframely snippets in your new doc will work well for previews in the editor, but may be suboptimal on the actual site. HTML produced by our APIs is better optimized for sites bloated with ads (=most sites). Also, we optimize codes to avoid flickering of the pages: individual media reserves its correct DOM space even before it starts rendering, so that page doesn't jerk around and general user experience is better.
  • Also, caches of HTML codes need to be updated from time-to-time. At the very least to detect when rich media is no longer available, for example.
  • Finally and most importantly, sites may require different output for the articles depending on the environment. Say, markup is different for Google AMP or Facebook Instant Articles. Or simply for the site's mobile app (say, React Native...). Most sites have multiple representations these days.

BTW, do I get it right that semantic output lets devs/sites to overwrite HTML codes for all publishers, including the defaults ones that have a hardcoded HTMLs included into the package? We'd appreciate a special mention for this point in the doc too.

The working code example for server-code, if anything, can be a simple request of URL to https://ckeditor.iframe.ly/api/oembed (yep, now with ssl), and then using the returned html.

Cheers!

@Reinmar
Copy link
Member Author

Reinmar commented Aug 31, 2018

Hi Ivan! Thanks for validating our docs. I really appreciate that :)

So, to check whether I understand correctly – my first guess was that you recommend to replace saved <oembed> tags with HTML provided by Iframely on one's backend (PHP/Java/etc), not on the frontend (on the final website)? That'd be understandable when considering performance aspects but I'm not sure I understand the other points (why is it better for aspect-ratios or different outputs).

Therefore, my second guess wass that you meant that we should never save HTML previews into the database or that these preview's should only be used in the editor itself, not on the final website (in which case saving them makes little sense). That would better explain the points with aspect ratio or the different outputs.

So, let me know which guess was correct :)

Regarding why we allow saving that preview HTML to a database – we made that for convenience reasons. Many developers may not have the time and resources to implement Iframely and then the previews are sufficient. However, I agree they are of far lower quality than what one can get from your service. I think we can still change the default settings (e.g. to not output HTML previews to the database by default) and change the documentation accordingly. But we'll need to find the right balance between easy to install but lower quality and harder to use but as good as it can be.

@iparamonau
Copy link

What I meant is that people don't read the documents. They just skim through examples and get ideas from it. After they think they understand, they will dig deeper into the doc, to confirm their opinionated understanding. In case of Iframely examples, they may jump to suboptimal conclusions. Not saying the examples are not great - they might actually be the best ones for number of use-cases. Just saying that those examples should not be emphasized as the only ones.

Therefore, I am supporting the idea of this GitHub issue: create a separate section with an example for the server-side renders. And explain in more details why it might be needed.

So that it triggers more thoughts on reader's side. Such as these important ones:
a) how they are going to refresh the HTML codes. Or migrate all of their CMS from CKEditor 4.0 format (which is the same or similar I believe, btw)
b) do they even need different HTML codes for different environments such as Google AMP, Facebook Instant Articles or mobile apps?

Obviously, both semantic and non-semantic output can be replaced on the server or on the client alike. From our experience, it's more important that devs (or better yet - product owners) understand that HTML from built-in previews is not necessarily the same the end-users will or should see. It will save us all a bunch of support time if we make sure people understand the difference. Adding a server-side section to the doc will make this distinction obvious and difficult to pass by.

@Reinmar Reinmar self-assigned this Sep 25, 2018
Reinmar referenced this issue in ckeditor/ckeditor5-media-embed Oct 9, 2018
…fact that unfurling urls on the client-side is suboptimal. See #15.
@Reinmar
Copy link
Member Author

Reinmar commented Oct 9, 2018

Thanks @iparamonau, once again. Based on your feedback and some more thoughts that we had, we changed one important thing in this feature – it is going to produce a semantic output (no previews) by default. The "include previews in data" mode is an option and I'd say that not a very appealing one. This, together with some changes that we've done in the documentation, should hopefully draw people's attention to other options and raise their general awareness.

One more thing that I noticed in your documention is:

Always use hosted iFrames as rendering engine or switch to Web Components in browsers that support it.

What do you mean by Web Components here?

@Reinmar Reinmar removed their assignment Oct 9, 2018
@iparamonau
Copy link

@Reinmar by default, Iframely will use import of templates and perhaps shadow DOM in Chrome to bulk-insert some embeds such as, say, our summary cards.

Basically, our HTML for such widgets is <div ...><a href="..." data-ifamely-url="..."></a></div> + our <script src="...embed.js"> instead of plain iFrame that allows us for speedier inserts in the browsers that support it. But users can disable it and will always get iFrame right away. Doesn't affect CKEditor as far as I can tell.

The changes to the document itself look good. But I believe previewing all rich media in the editor is also important in order to keep embeds user-friendly. Let us discuss it here internally, perhaps we'll be able to come up with proposal on that.

@marcellokabora
Copy link

marcellokabora commented Mar 10, 2019

I create a pipe to handle this oembed:

import { Pipe, PipeTransform } from '@angular/core';
@Pipe({
  name: 'oembed'
})
export class OembedPipe implements PipeTransform {
  transform(htmlContent: any): any {
    const oembed = htmlContent.split('</oembed>');
    let body = '';
    oembed.forEach((item, index) => {
      body += oembed[index] + '</oembed>';
      const oembed1 = item.split('url="')[1];
      if (oembed1) {
        const oembed2 = oembed1.split('">')[0];
        if (oembed2) {
          const youtube = oembed2.split('https://www.youtube.com/watch?v=')[1];
          if (youtube) {
            body += '<div class="iframe-container"><iframe src="https://youtube.com/embed/' + youtube + '" frameborder="0"; scrolling="no"; allow="accelerometer; autoplay; encrypted-media; gyroscope; picture-in-picture" allowfullscreen></iframe></div>';
          }
        }
      }
    });
    return body;
  }
}

@Big-Silver
Copy link

Big-Silver commented Jun 5, 2019

I replaced the ombed to iframe using Regex.
It's just for Youtube and for vimeo case maybe need to add video tag instead of iframe.
Please check this jsfiddle.
https://jsfiddle.net/2vkc0ar8/

@mlewand mlewand transferred this issue from ckeditor/ckeditor5-media-embed Oct 9, 2019
@mlewand mlewand added this to the backlog milestone Oct 9, 2019
@mlewand mlewand added status:discussion type:docs This issue reports a task related to documentation (e.g. an idea for a guide). package:media-embed labels Oct 9, 2019
@tony
Copy link
Contributor

tony commented Mar 12, 2021

I was caught off guard by this - I didn't realize I needed to also have a separate rendering system.

I thought if YouTube showed inside the Editor, things would just work at the get go.

I agree with @iparamonau on people not reading all the documentation.

The sheer size of the docs, the fact that previewing works flawlessly - but this is illusory without something to render it. 🤔

@tony
Copy link
Contributor

tony commented Mar 13, 2021

One more thing: A web component would need to have a hyphen, WICG/webcomponents#658

As of a127c97, we use <oembed>:

viewElement = writer.createEmptyElement( 'oembed', attributes );

So we need to switch to something with a hyphen, e.g. <o-embed>, which should work

@tony
Copy link
Contributor

tony commented Mar 13, 2021

Here's an <o-embed> web component, works with YouTube, Vimeo, Dailymotion and Spotify.

License MIT, TypeScript:

Examples:

Right now you can have this working by replace 'oembed' with o-embed in two spots in the media-embed module. Then import this module wherever you render <o-embed> tags.

Update (via #9418, aefc6a2, 27.1.0): config.mediaEmbed.elementName can be set to 'o-embed':

{
  mediaEmbed: {
    elementName: 'o-embed'
  }
}

If you just want the regexes / matching / etc: @social-embed/lib (docs)
Examples: https://codepen.io/attachment/pen/VwPPrNq, https://jsfiddle.net/gitpull/pcLagbsm/, https://codepen.io/attachment/pen/poRRpdp?editors=0010

@bkalafat
Copy link

I create a pipe to handle this oembed:

import { Pipe, PipeTransform } from '@angular/core';
@Pipe({
  name: 'oembed'
})
export class OembedPipe implements PipeTransform {
  transform(htmlContent: any): any {
    const oembed = htmlContent.split('</oembed>');
    let body = '';
    oembed.forEach((item, index) => {
      body += oembed[index] + '</oembed>';
      const oembed1 = item.split('url="')[1];
      if (oembed1) {
        const oembed2 = oembed1.split('">')[0];
        if (oembed2) {
          const youtube = oembed2.split('https://www.youtube.com/watch?v=')[1];
          if (youtube) {
            body += '<div class="iframe-container"><iframe src="https://youtube.com/embed/' + youtube + '" frameborder="0"; scrolling="no"; allow="accelerometer; autoplay; encrypted-media; gyroscope; picture-in-picture" allowfullscreen></iframe></div>';
          }
        }
      }
    });
    return body;
  }
}

Do you have twitter version :)

@pomek pomek removed this from the backlog milestone Feb 21, 2022
@CKEditorBot
Copy link
Collaborator

There has been no activity on this issue for the past year. We've marked it as stale and will close it in 30 days. We understand it may be relevant, so if you're interested in the solution, leave a comment or reaction under this issue.

@tony
Copy link
Contributor

tony commented Nov 1, 2023

Fine to keep this open

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package:media-embed status:discussion type:docs This issue reports a task related to documentation (e.g. an idea for a guide).
Projects
None yet
Development

No branches or pull requests

9 participants