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

How to configure the feature? #2733

Closed
oleq opened this issue Aug 20, 2018 · 4 comments
Closed

How to configure the feature? #2733

oleq opened this issue Aug 20, 2018 · 4 comments
Labels
package:media-embed status:discussion type:question This issue asks a question (how to...).
Milestone

Comments

@oleq
Copy link
Member

oleq commented Aug 20, 2018

ATM there are 2 config options: config.mediaEmbed.semanticDataOutput (more in here BTW) and config.mediaEmbed.media.

MediaEmbedEditing defines the later in its contructor editor.config.define( 'mediaEmbed', { defaults } ) providing tons (well, just a few 😛) of defaults like YouTube, Vimeo, Facebook, Spotify, etc.

Problems:

  • Most likely developers won't use our defaults. They want to choose precisely what should be allowed and what shouldn't land in the data.
  • If they go from scratch, that's OK....
  • But if they want to, let's say, re-use some of our defaults + add something custom, or even worse, limit the number of defaults, they're gonna have a problem...
  • Our config API provides no way to change the defaults. AFAIR it allows extending editor.config.define if the config property is an object, but there's no way to get rid of some defined media providers or start building their own config on top of defaults because when the editor is configured (Editor.create( el, cfg )) there's no way to access defaults or any other data source like editor.plugins.get( 'MediaEmbed' ).defaultProviders or whatever. They don't exist yet.
  • Shouldn't it be config.mediaEmbed.providers BTW?

We've never met this problem before because so far there was no other feature that required such a complex and extensive configuration. That may require some changes in our approach to the Config class, TBH I'm not sure what to do about that.

@pjasiun
Copy link

pjasiun commented Aug 21, 2018

Maybe it should be configurable through an event? The default behavior could be a default listener, but one will be able to add extra cases or overwrite it completely using the higher priority.

@oleq
Copy link
Member Author

oleq commented Aug 22, 2018

An event fired by what? Because there's nothing to attach a listener to until the editor is created. And when it's created, the configs have long been parsed, the editing working and the data loaded so it's too late. How could a developer configure it using the event system then?

Or maybe I missed something in your comment?

@Reinmar
Copy link
Member

Reinmar commented Aug 22, 2018

The starting point: the feature comes with a list of default providers (some with previews and some without previews).

What developers need:

  1. Ability to add more providers.

    mediaEmbed.extraProviders

  2. Ability to override the entire list of providers.

    mediaEmbed.providers

  3. Ability to disable some default providers.

    mediaEmbed.removeProviders

  4. I want only semantic output in the data (but still previews of previewable things in the editor).

    mediaEmbed.semanticDataOutput

  5. From the default list of providers, enable only previewable ones (subset of 3.). Do we need it as a separate option? Perhaps we can simply document how to use removeProviders

    mediaEmbed.removeProviders = [ 'instagram', 'twitter', ... ] // list of them in the docs

  6. I want no previews in the editor (in the editing view) cause it may be confusing that some things are displayed with previews and some not:

    mediaEmbed.disablePreviews

To make the config.removeProviders option work we also need to make sure that providers are named. It'd be good if we could make config.mediaEmbed.providers but then overriding this property via our config would not work. So it needs to be an array. The name property can be optional so if you use extraProviders or override everything via providers you won't need to pass it.

editor.config.define( 'mediaEmbed', {
	providers: [
		{
			name: 'dailymotion',
			url: [
				/^dailymotion\.com\/video\/(\w+)/
			],
			html: id =>
				'<div style="position: relative; padding-bottom: 100%; height: 0; ">' +
					`<iframe src="https://www.dailymotion.com/embed/video/${ id }" ` +
						'style="position: absolute; width: 100%; height: 100%; top: 0; left: 0;" ' +
						'frameborder="0" width="480" height="270" allowfullscreen allow="autoplay">' +
					'</iframe>' +
				'</div>'
		},

		{
			name: 'spotify',
			url: [
				/^open\.spotify\.com\/(artist\/\w+)/,
				/^open\.spotify\.com\/(album\/\w+)/,
				/^open\.spotify\.com\/(track\/\w+)/
			],
			html: id =>
				'<div style="position: relative; padding-bottom: 100%; height: 0; padding-bottom: 126%;">' +
					`<iframe src="https://open.spotify.com/embed/${ id }" ` +
						'style="position: absolute; width: 100%; height: 100%; top: 0; left: 0;" ' +
						'frameborder="0" allowtransparency="true" allow="encrypted-media">' +
					'</iframe>' +
				'</div>'
		},
	]
} );

PS. Should we rename html to preview? I'm hesitating. The "preview" makes sense in the editor, but in the data it's not a preview – it's the final piece of the data. So here html makes more sense.

@oleq
Copy link
Member Author

oleq commented Aug 24, 2018

Closed in ckeditor/ckeditor5-media-embed#2.

@oleq oleq closed this as completed Aug 24, 2018
@mlewand mlewand transferred this issue from ckeditor/ckeditor5-media-embed Oct 9, 2019
@mlewand mlewand added this to the iteration 20 milestone Oct 9, 2019
@mlewand mlewand added status:discussion type:question This issue asks a question (how to...). package:media-embed labels Oct 9, 2019
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:question This issue asks a question (how to...).
Projects
None yet
Development

No branches or pull requests

4 participants