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 embed handler for gfycat. #1136

Merged
merged 6 commits into from May 19, 2018

Conversation

Projects
None yet
3 participants
@miina
Copy link
Collaborator

miina commented May 9, 2018

No description provided.

miina added some commits May 9, 2018

*/
public function test__conversion( $source, $expected ) {
if ( ( version_compare( get_bloginfo( 'version' ), '4.9', '<' ) ) ) {
$this->markTestSkipped( 'Gfycat is not supported in this WP version.' );

This comment has been minimized.

@miina

miina May 9, 2018

Collaborator

Added skipping tests since it looked like amp-iframe was not created in WP 4.8 and WP 4.7 and some tests failed, however, after testing locally, not sure it's related to WP version. Thought that perhaps the tests failed due to PHP version. Any thoughts on what might fail, could wp_parse_url used here differ in previous versions?

This comment has been minimized.

@kienstra

kienstra May 10, 2018

Collaborator

Hi @miina, good question.

Could you try adding this part of the setUp method from AMP_SoundCloud_Embed_Test to AMP_Gfycat_Embed_Test?

That worked in the testing PR #1138 that I opened, even without $this->markTestSkipped().

The failed build that you linked to used WordPress version 4.7. When I set my local to that version, the test here failed (without any modification). Strangely, the code works fine on the front-end. Just not in the tests in that case.

@kienstra
Copy link
Collaborator

kienstra left a comment

Looks Good, A Few Points

Hi @miina,
Thanks, the code looks really good. I responded to your question about the tests, and made a comment about the layout.

But this looks good, and is pretty close to being ready to merge.

/**
* Regex matched to produce output amp-gfycat.
*
* @const string

This comment has been minimized.

@kienstra

kienstra May 10, 2018

Collaborator

Strangely, even constants should have a @var tag. I just recently learned this. But it's not a blocker.

}
/**
* Filter oEmbed HTML for Meetup to prepare it for AMP.

This comment has been minimized.

@kienstra

kienstra May 10, 2018

Collaborator

Should this be for Gfycat?

'width' => $attr['width'],
'height' => $attr['height'],
'data-gfyid' => $data_gfyid,
'layout' => 'responsive',

This comment has been minimized.

@kienstra

kienstra May 10, 2018

Collaborator

How about a layout of intrinsic here? That layout is like responsive, in that it expands to the size of its container.

But it always has at least the dimensions in the width and height.

When left or right-aligning the Embed block, this <amp-gfycat> gets wrapped in a <figure>.

right-aligned-gutenberg

But that <figure> doesn't look to have width or height of its own, so the layout="responsive" value causes the <amp-gfycat> to not appear.

With layout="intrinsic", the element appears:

right-aligned-front-end

We're already using layout="intrinsic" as defaults for <amp-img> and <amp-iframe>

*/
public function test__conversion( $source, $expected ) {
if ( ( version_compare( get_bloginfo( 'version' ), '4.9', '<' ) ) ) {
$this->markTestSkipped( 'Gfycat is not supported in this WP version.' );

This comment has been minimized.

@kienstra

kienstra May 10, 2018

Collaborator

Hi @miina, good question.

Could you try adding this part of the setUp method from AMP_SoundCloud_Embed_Test to AMP_Gfycat_Embed_Test?

That worked in the testing PR #1138 that I opened, even without $this->markTestSkipped().

The failed build that you linked to used WordPress version 4.7. When I set my local to that version, the test here failed (without any modification). Strangely, the code works fine on the front-end. Just not in the tests in that case.

miina added some commits May 10, 2018

@miina

This comment has been minimized.

Copy link
Collaborator

miina commented May 10, 2018

@kienstra Thanks for reviewing and taking the time to create a separate PR for the tests fix! The issues should be addressed now.

@kienstra
Copy link
Collaborator

kienstra left a comment

Approved

Hi @miina,
Thanks, this looks good.

@kienstra

This comment has been minimized.

Copy link
Collaborator

kienstra commented May 10, 2018

Request To Merge

Hi @westonruter,
When you have a chance, could you please merge this pull request? @miina's work here looks good.

@westonruter westonruter added this to the v1.0 milestone May 11, 2018

@westonruter
Copy link
Member

westonruter left a comment

This doesn't seem to work quite right for me. I went to https://gfycat.com/gifs/detail/WebbedBossyCollardlizard and I clicked on the share icon and got the URL https://gfycat.com/webbedbossycollardlizard

If I embed that URL into the non-AMP post then it is successfully embedded as an oEmbed. If I view the post in AMP, then it gets displayed as an amp-iframe instead of amp-gfycat as I'd expect.

public function filter_embed_oembed_html( $return, $url, $attr ) {
$parsed_url = wp_parse_url( $url );
if ( false !== strpos( $parsed_url['host'], 'gfycat.com' ) ) {
if ( preg_match( '/width=(?:"|\')(\d+)(?:"|\')/', $return, $matches ) ) {

This comment has been minimized.

@westonruter

westonruter May 11, 2018

Member

Might want to adjust this to be like '/width=["\']?(\d+)/' since the quote marks would be optional in HTML.

}
if ( preg_match( '/height=(?:"|\')(\d+)(?:"|\')/', $return, $matches ) ) {
$attr['height'] = $matches[1];
}

This comment has been minimized.

@westonruter

westonruter May 11, 2018

Member

Shouldn't this bail if the width and height are empty? In that case we wouldn't be able to build a valid amp-gfycat since the dimensions wouldn't be there.

This comment has been minimized.

@miina

miina May 14, 2018

Collaborator

Added checking for height within e1d29a8 and using fixed-height for layout if only width is missing.

$attr['height'] = $matches[1];
}
$pieces = explode( '/detail/', $parsed_url['path'] );

This comment has been minimized.

@westonruter

westonruter May 11, 2018

Member

This isn't taking into account the oEmbed URLs like https://gfycat.com/webbedbossycollardlizard

@westonruter

This comment has been minimized.

Copy link
Member

westonruter commented May 11, 2018

I'm curious: the amp-iframe and the amp-gfycat components seemed to render exactly the same. Why is the amp-gfycat component even necessary?

@miina

This comment has been minimized.

Copy link
Collaborator

miina commented May 11, 2018

@westonruter This is an example output with amp-gfycat (with URL http://gfycat.com/gifs/detail/webbedbossycollardlizard, as you mentioned in comments there's an issue with the logic currently that expects the URL to be in format ...gfycat.com/gifs/detail/[ID], should allow gfycat.com/[ID], too.):
screen shot 2018-05-11 at 8 26 00 pm

And this without:
screen shot 2018-05-11 at 8 26 54 pm

Are you thinking that amp-gfycat isn't necessary at all since amp-iframe is already generated for gfycat and since there might be no difference for the user?

@westonruter

This comment has been minimized.

Copy link
Member

westonruter commented May 11, 2018

Are you thinking that amp-gfycat isn't necessary at all since amp-iframe is already generated for gfycat and since there might be no difference for the user?

Right. I'm confused as to why the amp-gfycat component exists when an amp-iframe seems to work just as well? The main thing that it does is allow you to easily provide just a gfyid to construct the embed. Additionally, there is an noautoplay attribute which can be used but it merely adds an ? autoplay=0 to the iframe'd URL:

https://github.com/ampproject/amphtml/blob/04f3484ee0a4506a4cd205b373d7be3763d02649/extensions/amp-gfycat/0.1/amp-gfycat.js#L132-L136

That noautoplay attribute seems to be the main benefit here from a WordPress context, since gfycat isn't forwarding along the autoplay query parameter to the oEmbed response. But if not using oEmbed, then it could just be constructed with amp-iframe.

@westonruter

This comment has been minimized.

Copy link
Member

westonruter commented May 11, 2018

Correction, it looks like the automatic addition of the placeholder is more of a key aspect of the component: https://github.com/ampproject/amphtml/blob/04f3484ee0a4506a4cd205b373d7be3763d02649/extensions/amp-gfycat/0.1/amp-gfycat.js#L84-L108

So yeah, I think we should go ahead with the custom handler you've worked on here. But it just needs to be finished off.

miina and others added some commits May 14, 2018

return $return;
}
$layout = 'intrinsic';

This comment has been minimized.

@westonruter

westonruter May 19, 2018

Member

This causes an AMP validation error:

The specified layout 'INTRINSIC' is not supported by tag 'amp-gfycat'.

@westonruter westonruter merged commit 370ca98 into develop May 19, 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/gfycat-handler branch May 19, 2018

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