Following the discussion in #10467 and #9838 , we would like to introduce a list of supported embed URLs for which we automatically read the iframe's resize postMessage and interpret it then resize our iframe's height accordingly.
As determined in #10467 , unfortunately most of the popular iframe embeds don't send any resize message of any sort. However, Storify is one of the most requested embeds and it sends a resize message in this format:
MessageEvent {isTrusted: true, data: "
{
"method":"resize",
"value":6809,
"sourceName":"sacb…-relievers-sean-doolittle-ryan-madson-traded-to"}",
origin: "http://storify.com",
lastEventId: "",
source: Window…
}
For this GFI, you will be implementing an adapter that reads the message and updates the iframe's height accordingly.
amp-iframe currently requires a specific resize message to be able to resize itself per the embed's request. The resize postMessage has to be in this format:
window.parent.postMessage({
sentinel: 'amp',
type: 'embed-size',
height: document.body.scrollHeight
}, '*');
Of course, most websites won't support this format unless they have an AMP-specific implementation.
postMessage listener in amp-iframethis.updateSize_()postMessage)src/base-element.jsCloses Issue #10661 in the description and request a review from @wassgha.Once approved, your changes will be merged. ⚡⚡⚡Congrats on making your first contribution to the AMP Project!⚡⚡⚡ You'll be able to see it live across the web soon!
Thanks, and we hope to see more contributions from you soon.
If you have questions ask in this issue or on your Pull Request (if you've created one) or see the How to get help section of the Getting Started guide.
@aghassemi @mrjoro Does this qualify as a GFI or is it too much work?
I think it's fine for to have GFIs that cover a range of backgrounds--e.g. some people may be very advanced web developers who haven't contributed to open source before, so they use GFIs as a way to do that. Check out the docs for creating a GFI and make sure to use the template.
You can fill in the "What you need to know" section with background knowledge someone should have when tackling the GFI, and break down the tasks into work that people who have that background knowledge but who haven't worked on AMP before will be able to tackle (with pointers if needed).
@wassgha If we already had one adapter, this would be a great First Timer GFI. (idea for subsequent GFIs)
Still fine as a GFI, maybe just add a "Implementation Strategy" section and include a bit more details (and move the existing implementation-related bullet points from the Step by step into the "Implementation Strategy" and just add a "Follow the implementation strategy" bullet in the Step by step section)
hey, i'd like to claim this issue, it'll be my first OSS contribution even though i've been working as a front-end developer for years.
Great! @kboluk I sent you an invite to join ampproject; once you accept we can assign this issue to you.
Hey @kboluk is there any progress on this issue?
Since there haven't been updates in a while we'll make this available for anyone who is interested in working on a good first issue.
@wassgha @mrjoro I would like to claim this issue as i reviewed it and i feel i can do it.
Great ! @mrjoro will invite you :)
@wassgha @FadySamirSadek is there any updates on this? If not, I'd like to claim it.
Update:
Just visiting storify, they announced their end-of-life on December 12th.
(...) Unfortunately, we are no longer allowing new users to sign up and will end existing users' access to Storify.com on May 16, 2018.
Storify End-of-Life
Additionally, I see it will be a paid feature of Adobe's Livefyre, they call it storify 2.
Using google amp with storify 2
should we update the target embed to something else for this specific GFI?
@juanlizarazo I am stuck so I do not mind if you claim it but please keep me updated because I want to know how you did it
@aghassemi can you comment on @juanlizarazo's question when you get a chance?
After thinking about this a for a bit and looking at several embed types, I think we should pivot and create a amp-embedly component instead as it would cover more cases and is actually simpler as a GFI than this.
@juanlizarazo would you be interested in creating an <amp-embedly url=<whatever embed.ly supports> component for AMP?
@aghassemi definitively!
Awesome, please see https://github.com/ampproject/amphtml/issues/12908. It is a bit more involved than other good first issues in AMP, so depending on your comfort level you may want to start with something else first and then move on to it.
Thanks @aghassemi! I can get started on it this weekend.
@aghassemi it sounds like we aren't going to go forward with this issue now and we should close it?
Closing in favour of #12908
Most helpful comment
hey, i'd like to claim this issue, it'll be my first OSS contribution even though i've been working as a front-end developer for years.