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

Add support for dai.ly urls in MediaEmbedEditing #15388

Merged
merged 1 commit into from
Nov 27, 2023

Conversation

Kocal
Copy link
Contributor

@Kocal Kocal commented Nov 22, 2023

Suggested merge commit message (convention)

Feature: Add support for short Dailymotion URLs (dai.ly) in media-embed.


Additional information

Short Dailymotion URLs can be found in you share the video through the dedicated sharing button:

Example: https://www.dailymotion.com/video/x8ks8y1
image
image

@Witoso
Copy link
Member

Witoso commented Nov 22, 2023

I'm not sure if we want to add more providers to the plugin except the most popular, as it's adds the maintenance cost. @Reinmar, thoughts?

@Kocal
Copy link
Contributor Author

Kocal commented Nov 22, 2023

I can understand, but I didn't find anything in the documentation, like extending an existing media provider to add more URLs

@Witoso
Copy link
Member

Witoso commented Nov 22, 2023

There's extending, removing, overriding. I am not sure if I understand your case.

@Kocal
Copy link
Contributor Author

Kocal commented Nov 22, 2023

It's more something like extending a given mediaProvider to add another supported URL.

By doing something like:

{ 
  mediaEmbed: {
    extraProviders: [
      { 
        name: 'dailymotion',
        url: [/^dai.ly\/(\w+)/],
      }
    ]
  }
}

I was expecting my dailymotion configuration to be merged with the one from the MediaEmbed plugin itself, so maintaining would be easier/safer in our end-user side:

But it didn't work as I thought, I had to duplicate the dailymotion configuration from the MediaEmbed plugin, and add it like this:

{ 
  mediaEmbed: {
    extraProviders: [
      { 
        name: 'dailymotion',
        url: [/^dai.ly\/(\w+)/],
        html: (match) => {
          const id = match[1];
          return (
            '<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>'
          );
        },
      }
    ]
  }
}

@Witoso
Copy link
Member

Witoso commented Nov 22, 2023

Ok, gotcha. I forgot we actually had dailymotion 🤦  @arkflpc could you take a look and merge it?

@Witoso Witoso added the squad:core Issue to be handled by the Core team. label Nov 27, 2023
@arkflpc arkflpc merged commit da8c3ac into ckeditor:master Nov 27, 2023
@Kocal Kocal deleted the patch-1 branch November 27, 2023 20:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
squad:core Issue to be handled by the Core team.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants