-
Notifications
You must be signed in to change notification settings - Fork 182
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
Try to integrate the amp-hulu tag #162
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like there is a check missing whether the iframe is actually a hulu embed.
@sebastianbenz Oh that is a good point. Thank you for the feedback. To be honest I got stuck with the validation. Just this new pass, causes a disallowed amp-hulu tag. Given that I thought one has to update the spec file, by updating the forked amphtml repo, merging in the changes from google upstream, and finally building the This though causes some of those assertion errors:
As far as I see this problem can be seen in the spec file:
As far as I understand this should actually though use |
@dawehner have a look at the vimeo sample: https://github.com/Lullabot/amp-library/blob/master/src/Pass/IframeVimeoTagTransformPass.php#L47 They match the iframe URL to a regex to check if the iframe is a vimeo embed. You'd need to do something similar for hulu. |
Thank you for letting me know. This is blocked until we don't get the new generated validator code, because the one in HEAD doesn't yet support |
b2fc1fa
to
5265d55
Compare
@sebastianbenz It would be super cool if you could help me out onhttps://github.com/Lullabot/amphtml/pull/1 |
Note: I managed to generate the validator and tweak it a little bit. It outputs all uppercase tags, which doesn't work with all the test coverage. There are remaining failures left, but I guess we can fix them by tuning the validator a bit more? To be honest that sounds wrong. |
Tuning the validator sounds wrong. What's needed to make it work? Thanks btw for going through the pain of migrating to a new validator version! To be honest - I'm no real help here as I haven't been involved in developing amp-library. |
He, I did not expected anything else :)
The version which is in master of the amp-library doesn't include the 'amp-hulu' tag yet as allowed tag. The new exported validator though then contains uppercase tags, so I had to manually tune them to no longer do that. Still, there are a lot of test failures. Do you think that manually putting in the amp-hulu tag and its JS would cut it? |
Regarding the uppercase tags - is this something we can handle in the php
validator runtime?
The other test failures - can they be fixed by updating the amp-library
transformations?
…On Fri, Feb 17, 2017 at 4:57 PM Daniel Wehner ***@***.***> wrote:
Tuning the validator sounds wrong
He, I did not expected anything else :)
What's needed to make it work?
The version which is in master of the amp-library doesn't include the
'amp-hulu' tag yet as allowed tag. The new exported validator though then
contains uppercase tags, so I had to manually tune them to no longer do
that. Still, there are a lot of test failures.
Do you think that manually putting in the amp-hulu tag and its JS would
cut it?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#162 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAXOOAxxR6xAQydIWdLrD-KXJI2UF-fuks5rddFsgaJpZM4L66Fh>
.
|
I guess we could alter the initialized objects when we create them.
I haven't looked into them too much. One thing I noticed is that it no longer stripped out submit buttons. |
@dawehner I can see that changing the spec breaks a lot of functionality in the library. When you generated the spec file, what version of the amphtml project did you use? I think it'd be good to use the current running version of the AMP library - https://github.com/ampproject/amphtml/tree/1487358851767 That way, we know that the code is stable. What I see pop up a lot is amp4ads, which is still a draft and should only be used for creatives (ads).
TL;DRI think the biggest win for you would be to start by fixing the amp4ads. That's going to remove most of the failures, allowing you to focus on what's left. |
Thank you for the guidance!
This is what I did. I merged the master of amphtml into the fork from lullabot, see dawehner/amphtml@bc0812e I'll try to work on the failures. Thank you |
I fixed the amp4ads validation as well as many other instances of wrong expactations. We are down to 16 failing tests now. |
I fixed a couple of more failures. There are a couple of items, like the youtube failures which are at the moment a bit challenging. There are other ones I'm not sure what to do about them, like nested |
How i can help for get this done? i think we all need re make the validator.php for other integrations and contribute our needs of new components and the validator branch is very far behind master of amphtml. If we can solve all the issues here or in a new issue it would be very nice to keep updated this library. @sigginet or @sebastianbenz can we talk about it? |
I'm looking through this..
I need to head out now, but I hope this helps a bit to move this PR forward :) |
Sorry for not continueing the work here, other priorities got set. I hope I can get back to it |
This PR tries to integrate the amp-html by checking HULU embedds.
It seems to be though that we have to update the generator spec file first ...