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 the auto embed (sub)feature #2731

Closed
oleq opened this issue Aug 20, 2018 · 17 comments · Fixed by ckeditor/ckeditor5-media-embed#27
Closed

Implement the auto embed (sub)feature #2731

oleq opened this issue Aug 20, 2018 · 17 comments · Fixed by ckeditor/ckeditor5-media-embed#27
Assignees
Labels
package:media-embed type:feature This issue reports a feature request (an idea for a new functionality or a missing option).
Milestone

Comments

@oleq
Copy link
Member

oleq commented Aug 20, 2018

Just like in v4 we probably want to implement the Auto Embed. Long story short, it's all about upcasting the pasted urls from the clipboard straight to embedded media. Check out the v4 demo to learn more.

@Reinmar
Copy link
Member

Reinmar commented Aug 28, 2018

@pomek, I'm not sure if you'll find the time to work on this, but I'm assigning it to you anyway. Let's talk once you'll finish the configuration task.

@Reinmar
Copy link
Member

Reinmar commented Aug 31, 2018

I'm wondering about the ability to undo the "auto embed" action. It's clear that there must be such ability because you might've wanted to paste such a link. However, there's a question how quickly the link should be embedded.

If we go for immediately the user won't even see that a link was inserted. That may make it less clear what happened.

If we'd wait a reasonable time before auto embedding the link we'll make it, I think, more clear, but then the feature becomes more complicated. Tests are getting tricky too.

PS. In CKEditor 4's demo you can see the link first, but I think it's coincidental there. It happens because the media embed in CKEditor 4 is asynchronous. CKEditor 5's media embed is synchronous so we won't get that behaviour out of the box. We need to code it.

cc @dkonopka @oleq @pjasiun

@wwalc
Copy link
Member

wwalc commented Aug 31, 2018

I'd say that the auto embed feature in CKEditor 4 should not be used as a guidance how to make it in CKEditor 5. For example it is annoying that in CKEditor 4 every URL is turned into oembed.

For example paste the following URL: https://allegro.pl/javascript-programowanie-zaawansowane-tomasz-jakut-i7448184547.html into the editor here: https://sdk.ckeditor.com/samples/mediaembed.html

If it was possible to define which providers should be handled automatically ("all" could be an option too, ofc), then it would be safer, I think.

@dkonopka
Copy link
Contributor

dkonopka commented Sep 3, 2018

For example it is annoying that in CKEditor 4 every URL is turned into oembed.

+1

IMHO, auto embed feature could be a configurable option (disabled by default), because I see some real use-cases when the user wants to insert plain text yt (or twitter) link instead of big, consuming space media-embed.

...or we should detect pasted URL and display some alert/confirm with the question about transforming it into media-embed.

@pomek
Copy link
Member

pomek commented Sep 3, 2018

...or we should detect pasted URL and display some alert/confirm with the question about transforming it into media-embed.

It sounds good (at least for me).

@Reinmar
Copy link
Member

Reinmar commented Sep 3, 2018

When I wrote about CKEditor 4 I didn't mean that we should copy this behaviour. It makes little sense indeed to autoembed every possible link like this. But that's not a case in how CKEditor 5's media embed works, because it's limited to a strictly defined set of URLs. In the future, we may also consider adding config.mediaEmbed.allowAutoEmbed {Array.<String>} which would allow limiting the number of providers which are taken into consideration by auto embedding.

What I wanted your comments on, instead, is what should happen when you paste an embeddable link (let's say YT). Should it be first shown in the editor and replaced with a widget in e.g. 1s? Or should the replacement happen immediately? Or should we go with some completely different direction, like e.g. manual replacement (an option in the link balloon?)?

Please remember, guys, that even automatic autoembedding must be still redoable. Which means that if a user wanted to paste a link and not autoembed it, he/she can still do that in such a scenario. This is how it works in CKEditor 4 and Medium.com. I don't know about any negative feedback about it (although, I haven't looked for it).

@pomek
Copy link
Member

pomek commented Sep 3, 2018

Another idea: insert the media element after pressing the Enter if an element (where is the selection) contains a valid URL to the media:

wrz-03-2018 16-01-03

@Reinmar
Copy link
Member

Reinmar commented Sep 4, 2018

Could you post a screencast of the autoembed on paste too? I know you're working simultaneously on two solutions.

@pomek
Copy link
Member

pomek commented Sep 4, 2018

Auto-embeding when a user pastes a valid URL to the media:

wrz-04-2018 10-45-41

@Reinmar
Copy link
Member

Reinmar commented Sep 4, 2018

And with a delay between paste and autoembedding? Let's say 500ms?

@pomek
Copy link
Member

pomek commented Sep 4, 2018

With the delay 500 ms:

wrz-04-2018 11-21-16

@Reinmar
Copy link
Member

Reinmar commented Sep 4, 2018

The 500ms delay and the "autoembed on Enter" both look fine to me. WDYT, guys?

@dkonopka
Copy link
Contributor

dkonopka commented Sep 4, 2018

👍 for "autoembed on Enter"

Autopaste with 500ms looks good too ATM, but in the future we could think about some confirmation ("Do you want to transform it into media-embed?")

@oleq
Copy link
Member Author

oleq commented Sep 11, 2018

I recently found myself waiting for Medium to upcast some pasted link for a while but all I had to do was press enter and it felt like a bug. I'm not sure anyone would simply paste an URL into the content and expect it to stay like that, so I'd go with the delay.

OTOH:

  • The problem with the delayed auto–embed I can see is a collision with the future auto–link feature. Some people may want to paste the URL, get it auto–linked and then edit its text to "Check out my video" instead of embedding the YT video directly in the content. That would make delayed auto–embed quite annoying.
  • Upcast on Enter seems simpler to implement as I found several issues when reviewing the 500ms PR which come down to the editor state changing during these 500ms (undo, typing, etc.) and some joyful errors along with the crashed editor.

@Reinmar
Copy link
Member

Reinmar commented Sep 11, 2018

I recently found myself waiting for Medium to upcast some pasted link for a while but all I had to do was press enter and it felt like a bug.

I forgot about this but I had the same exact issue when using Medium. I was waiting for the URL to get replaced and couldn't figure out why it doesn't work.

  • The problem with the delayed auto–embed I can see is a collision with the future auto–link feature. Some people may want to paste the URL, get it auto–linked and then edit its text to "Check out my video" instead of embedding the YT video directly in the content. That would make delayed auto–embed quite annoying.

For that we have the ability to undo just the "auto embed" action. Isn't that enough? It's like all kinds of "auto something" in Google Docs or the existing autoformatting feature in CKEditor 5.

  • Upcast on Enter seems simpler to implement as I found several issues when reviewing the 500ms PR which come down to the editor state changing during these 500ms (undo, typing, etc.) and some joyful errors along with the crashed editor.

Agree. That 500ms delay makes it more tricky because before final replacement safety checks need to be performed that the content we're touching didn't change in the meantime. Also, we shouldn't break the selection which may be in some other place (someone typed right after pasting that link).

We may also consider lowering that delay to a safer 100-200ms which may be enough to ensure that the user sees that the link got pasted but will reduce the chance that the user made more actions.

oleq referenced this issue in ckeditor/ckeditor5-media-embed Sep 14, 2018
Feature: Introduced the automatic media embedding. Closes #6.
@mlewand mlewand added this to the iteration 20 milestone Oct 9, 2019
@mlewand mlewand added status:confirmed type:feature This issue reports a feature request (an idea for a new functionality or a missing option). package:media-embed labels Oct 9, 2019
@mlewand mlewand transferred this issue from ckeditor/ckeditor5-media-embed Oct 9, 2019
@Im-Goutham
Copy link

How to disable this auto embed feature ?

@Mgsy
Copy link
Member

Mgsy commented Dec 13, 2019

How to disable this auto embed feature ?

There's no configuration option for disabling this feature, however, you can just import media embed plugin's dependencies without the auto embed (as you can see, it's a dependency of the media embed plugin):

import MediaEmbedEditing from '@ckeditor/ckeditor5-media-embed/src/mediaembedediting';
import MediaEmbedUI from '@ckeditor/ckeditor5-media-embed/src/mediaembedui';
import Widget from '@ckeditor/ckeditor5-widget/src/widget';

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package:media-embed type:feature This issue reports a feature request (an idea for a new functionality or a missing option).
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants