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

Not possible to embed Cloudup links #1233

Closed
iamgabrielma opened this issue Jun 17, 2017 · 7 comments
Closed

Not possible to embed Cloudup links #1233

iamgabrielma opened this issue Jun 17, 2017 · 7 comments
Assignees
Labels
[Type] Task Issues or PRs that have been broken down into an individual action to take

Comments

@iamgabrielma
Copy link
Contributor

iamgabrielma commented Jun 17, 2017

When pasting a Cloudup URL into the embed content block in a post, does not work and displays the message Sorry, we could not embed that content.

Steps to reproduce:
1 - Go to your Dashboard > Gutenberg > New post
2 - Click on the "+" add content button > Scroll down to "Embed" type of content > Click on "Cloudup"
3 - Paste a Cloudup URL and click Embed. Example URL: https://cloudup.com/cPWc0cgxWpy

Image of the issue

@jasmussen jasmussen added [Type] Bug An existing feature does not function as intended [Type] Question Questions about the design or development of the editor. and removed [Type] Bug An existing feature does not function as intended labels Jun 19, 2017
@jasmussen
Copy link
Contributor

Thought it was a bug, then relabelled because I realize this probably has to do with what oembeds WordPress supports. Which means it might be an upstream issue?

@aduth
Copy link
Member

aduth commented Jun 20, 2017

Cloudup is a supported oEmbed in core:

https://github.com/WordPress/WordPress/blob/3a19079/wp-includes/class-oembed.php#L96

I was able to verify this in my testing. Looking at the response received from the oEmbed proxy endpoint, it returns a valid result:

{
   "version":"1.0",
   "type":"photo",
   "title":"Screen Shot",
   "url":"https:\/\/cldup.com\/uKKtVAFSle-900x900.png",
   "provider_name":"Cloudup",
   "provider_url":"https:\/\/cloudup.com",
   "width":900,
   "height":346,
   "thumbnail_url":"https:\/\/cldup.com\/uKKtVAFSle-1200x1200.png",
   "thumbnail_width":1200,
   "thumbnail_height":462,
   "author_name":"Gabriel Maldonado"
}

However, looking at the implementation of the Embed block, it checks for the presence of an html property in the oEmbed response payload, which is not the case here as can be seen above.

https://github.com/WordPress/gutenberg/blob/5eceb50/blocks/library/embed/index.js#L128

We should consider enhancing the Embed block to render supported type properties like photo in this example.

Related: #816, cc @notnownikki

Aside: Cloudup is also registered as a standalone embed block variant:

https://github.com/WordPress/gutenberg/blob/5eceb50/blocks/library/embed/index.js#L204

@aduth aduth added [Type] Task Issues or PRs that have been broken down into an individual action to take and removed [Type] Question Questions about the design or development of the editor. labels Jun 20, 2017
@notnownikki
Copy link
Member

Ok I'll take a look at this and make a list of any other content types we might have to extend things for.

@notnownikki notnownikki self-assigned this Jun 20, 2017
@notnownikki
Copy link
Member

Fix for photo embeds is up for review here #1334

@aduth
Copy link
Member

aduth commented Jul 13, 2017

Fixed by #1334

@aduth aduth closed this as completed Jul 13, 2017
@swissspidy
Copy link
Member

There are also other types like video. Do these work?

@aduth
Copy link
Member

aduth commented Jul 13, 2017

Per 2.3.4.2 of the oEmbed spec, the video type must specify an html value, which is already supported by the embed block:

if ( html ) {
this.setState( { html, type } );

The link type could probably use better support though, since it mentions that it can omit both url and html properties. It's not clear to me what we'd expect to be shown for these types.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Type] Task Issues or PRs that have been broken down into an individual action to take
Projects
None yet
Development

No branches or pull requests

5 participants