Improve ae-embed plugin widget selection

[jbalsas]

Hey @ipeychev, this is a possible fix for #517.

We've changed the way the widgets are selected and handled to let CKEditor handle it in its own way, and figured out a different way to show the toolbar on them.

On top of that, we've also added a remove button to the embed edit button following the pattern on the link one so it is possible to remove a widget from the toolbar.

Thanks!

[jbalsas]

/cc @antoniopol06

[ipeychev]

Just started reviewing :)

Sent from GH.

[ipeychev]

Hey Chema,

This is really cool, but right now clicking on the button for adding an embedded media throws an exception? Please see the attached screencast.

Thanks,

[jbalsas]

Oh! Forgot to check that one! I was always pasting a url

I'm not at home right now, but I'll take a look as soon as a I get back!
El El dom, 22 may 2016 a las 18:26, Iliyan Peychev notifications@github.com
escribió:

Hey Chema,

This is really cool, but right now clicking on the button for adding an
embedded media throws an exception? Please see the attached screencast.

Thanks,
[image: bug]
https://cloud.githubusercontent.com/assets/78014/15455114/a9b56c32-204a-11e6-905e-158acf5cfcd3.gif

—
You are receiving this because you authored the thread.
Reply to this email directly or view it on GitHub
#524 (comment)

[ipeychev]

Great, the rest is cool!

One thing to consider - maybe it will make sense to allow people to set the size of the embedded content. Right now it takes the whole content width and there is no way to change it. In the same time, I'm not sure if we can make it perfect for every provider.
As I see, the response contains the name of the provider - "provider_name": "YouTube" or "provider_name": "Twitter" for example. Maybe we can add such class to the wrapping div, something like data-provider=YouTube, so people will be able to style it accordingly?

Thanks,

[jbalsas]

Hey Iliyan!

I've pushed one extra commit with some fixes for the remove button. I've refactored it so it better follows the same pattern as button-link-edit.

This has also made me realize we never got around to create tests for this feature, so I'll ask @antoniopol06 to work on that whenever we can.

[jbalsas]

Regarding the width and height of the embeds, I remembered you already brought this up on our initial discussion and I never followed on this. I'm really sorry about that 😢

I get the feeling that just a class might not be enough. Depending on the media content, it might be necessary to, for example, set the width and height attributes of the iframe, or maybe even request the embed again if the query supports dimensions as the output might be different.

How about we create an independent feature request for it so we can properly analyze it and tackle it as soon as we can?

[ipeychev]

Just started reviewing :)

Sent from GH.

[ipeychev]

Thank you, pull request merged! See changes here.

Sent from GH.

[ipeychev]

Hey Chema and Antonio,

Great, this was a cool fix which means it is time to release another minor version!

About setting the size of the embedded content, if you think the class won't be enough, then, sure - let's open a ticket to consider what can be done.

@jbalsas @antoniopol06