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

Improve support for Hulu & Imgur embeds #1218

Merged
merged 7 commits into from Jun 22, 2018

Conversation

Projects
None yet
3 participants
@miina
Collaborator

miina commented Jun 19, 2018

This PR aims to do the following:

  • Remove isSelected -- it's not needed anymore;
  • Make use of Fragment;
  • Change embed blocks category to embed;
  • Add AMP_Hulu_Embed_Handler;
  • Add AMP_Imgur_Embed_Handler;
@miina

This comment has been minimized.

Collaborator

miina commented Jun 19, 2018

Although the Hulu and Imgur embeds exist as default embed blocks as well, these are not working as expected, added AMP-specific blocks separately.

@miina miina requested a review from westonruter Jun 19, 2018

@westonruter

This comment has been minimized.

Member

westonruter commented Jun 19, 2018

Although the Hulu and Imgur embeds exist as default embed blocks as well, these are not working as expected, added AMP-specific blocks separately.

How are they not working? I think it would probably be better to extend the embed block to make sure that amp-hulu & amp-imgur components are used when the embed is rendered in AMP. This is particularly important now that the AMP blocks are only made available (for now) when the user has selected Native mode.

@miina

This comment has been minimized.

Collaborator

miina commented Jun 19, 2018

Copying from https://docs.google.com/spreadsheets/d/1P7x5M1ZU8dcs7O8PjwrQEKdPhZsTZW6Q0Q_flZjByz4/edit#gid=0:

In case of Hulu: Implemented, but doesn't display. Maybe an issue with the Hulu endpoint. Example URL: https://www.hulu.com/watch/771496

Adding an embed block with Hulu URL or just adding the link to post content is not displaying.

In case of Imgur:

Implemented, but depends un disallowed JS: //s.imgur.com/min/embed.js

When adding the existing Imgur block / adding an Imgur link to post content it doesn't display on the frontend.

Note that the embeds not working is not related to AMP / Gutenberg, these don't seem to work (not displaying on the front-end) using WP without Gutenberg / AMP plugin either.

Are you thinking that we should extend the existing Hulu and Imgur embed blocks to replace these with amp-hulu and amp-imgur instead? Thoughts?

@westonruter

This comment has been minimized.

Member

westonruter commented Jun 19, 2018

Are you thinking that we should extend the existing Hulu and Imgur embed blocks to replace these with amp-hulu and amp-imgur instead? Thoughts?

Yes, I think these probably need their own AMP_Hulu_Embed_Handler and AMP_Imgur_Embed_Handler classes just as there have been made for Instagram, Gfycat, etc.

@miina

This comment has been minimized.

Collaborator

miina commented Jun 19, 2018

Looks like the embed for Imgur isn’t working in 4.9>. Added wp_embed_register_handler for amp-imgur for lower versions, the embed handler is working now for lower versions as well, however the width and height are set to default instead of getting these from the video due to the embed not working.

@westonruter

This comment has been minimized.

Member

westonruter commented Jun 22, 2018

I'm having trouble getting Imgur to work. I'm trying with https://imgur.com/gallery/mlft1eR But all I get is:

image

On the frontend I get the same, just a bare URL that is not linked. There is no amp-imgur appearing.

For Hulu I am getting at least a black box for https://www.hulu.com/watch/771496:

image

Is this what you see?

@miina

This comment has been minimized.

Collaborator

miina commented Jun 22, 2018

However, tried with the URL that you tried with, I'm seeing the image on the backend:
screen shot 2018-06-22 at 10 41 55 am
But on the frontend I just see this:
screen shot 2018-06-22 at 10 44 09 am

The element gets replaced though:
screen shot 2018-06-22 at 10 43 52 am

  • On Hulu:
    I can see just the plain URL in the Gutenberg block and a black box on the frontend:
    screen shot 2018-06-22 at 10 48 42 am

The same Hulu URL shows a call to action to get a free trial when opening directly in browser, perhaps something has changed there?

@miina

This comment has been minimized.

Collaborator

miina commented Jun 22, 2018

Hmm, I just tested on another environment and now I'm seeing the same results as you are.

Maybe an option could be to extend the existing Imgur block to add AMP attribute controls and replace the embed field. This would essentially be replacing the whole existing block though, maybe the same as unregistering and adding new blocks instead with the same name.

Looks like the issue is the default Imgur embed not working and thus the embed handler doesn't run either. Is this something to report to core instead?

Thoughts?

@miina

This comment has been minimized.

Collaborator

miina commented Jun 22, 2018

Found a relevant topic on Imgur: https://core.trac.wordpress.org/ticket/42247

@westonruter

This comment has been minimized.

Member

westonruter commented Jun 22, 2018

For Imgur, interestingly if I make a request in my browser to https://api.imgur.com/oembed?maxwidth=600&maxheight=788&url=https%3A%2F%2Fimgur.com%2Fgallery%2Ff462IUj&dnt=1&format=json

Then I get:

{"version":"1.0","type":"rich","provider_name":"Imgur","provider_url":"https:\/\/imgur.com","width":600,"height":788,"html":"<blockquote class=\"imgur-embed-pub\" lang=\"en\" data-id=\"f462IUj\"><a href=\"https:\/\/imgur.com\/f462IUj\">Getting that beach body ready<\/a><\/blockquote><script async src=\"\/\/s.imgur.com\/min\/embed.js\" charset=\"utf-8\"><\/script>","author_name":"carrotsoup","author_url":"https:\/\/imgur.com\/user\/carrotsoup"}

But if I make a request from WordPress, I get:

image

Digging deeper I get “Permission denied” when I do:

curl -A 'WordPress/4.9.7-alpha-43298-src; https://src.wordpress-develop.test' 'https://api.imgur.com/oembed?maxwidth=600&maxheight=788&url=https%3A%2F%2Fimgur.com%2Fgallery%2Ff462IUj&dnt=1&format=json'

But if I do:

curl -A 'WordPress/4.9.7-alpha-43298-src; https://example.org' 'https://api.imgur.com/oembed?maxwidth=600&maxheight=788&url=https%3A%2F%2Fimgur.com%2Fgallery%2Ff462IUj&dnt=1&format=json'

Then I get:

{"version":"1.0","type":"rich","provider_name":"Imgur","provider_url":"https:\/\/imgur.com","width":600,"height":788,"html":"<blockquote class=\"imgur-embed-pub\" lang=\"en\" data-id=\"f462IUj\"><a href=\"https:\/\/imgur.com\/f462IUj\">Getting that beach body ready<\/a><\/blockquote><script async src=\"\/\/s.imgur.com\/min\/embed.js\" charset=\"utf-8\"><\/script>","author_name":"carrotsoup","author_url":"https:\/\/imgur.com\/user\/carrotsoup"}

For some reason they are forbidding requests that come from a “wordpress” domain. I've confirmed this by trying to embed an Imgur URL on WordPress.com and it fails, but if I embed an Imgur URL on WP Engine it succeeds. Strange! Anyway, that's indeed an issue on the Imgur side. If they don't want to serve content to WordPress.com (or *.wordpress.*) then that is up to them.

@westonruter westonruter added this to the v1.0 milestone Jun 22, 2018

@westonruter westonruter changed the title from Add AMP Hulu & Imgur blocks to Improve support for Hulu & Imgur embeds Jun 22, 2018

@westonruter westonruter merged commit 70711f0 into develop Jun 22, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@westonruter westonruter deleted the add/hulu_imgur_blocks branch Jun 22, 2018

@kienstra

This comment has been minimized.

Collaborator

kienstra commented Jul 6, 2018

Here's another Imgur URL that works well in the Gutenberg Imgur block:
https://imgur.com/a/ycEo4J1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment