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

AMP support? #59

Closed
1 of 4 tasks
benlk opened this issue Nov 30, 2018 · 6 comments · Fixed by #62
Closed
1 of 4 tasks

AMP support? #59

benlk opened this issue Nov 30, 2018 · 6 comments · Fixed by #62

Comments

@benlk
Copy link
Collaborator

benlk commented Nov 30, 2018

Fallback cases where Pym.js fails to load in the parent

For shortcodes:

  • allow the shortcode to be used as a [pym src=""]foo[/pym] that passes the contents to $content to be used as the innards of the placeholder div: <div id="pym_1" class="pym ">foo</div>

For blocks:

  • for blocks, the placeholder is a link to the embed

Specific to AMP

For whatever WordPress AMP plugins we want to support:

@benlk
Copy link
Collaborator Author

benlk commented Dec 7, 2018

https://amp-wp.org/documentation/playbooks/custom-shortcodes-and-widgets/ describes the function is_amp_endpoint() and amp_is_canonical() which we might use with some of the filters added recently to change the output embed code on the page. However, AMP iframe tags aren't 100% compatible with Pym; they use a different postMessage syntax. We'd need to link out to AMP's docs for how to initialize using the amp iframe code versus Pym.js, and we'd need to make these tests ourselves.

@philipjohn
Copy link

There's some discussion going on about this in the AMP Project

@claudiulodro
Copy link
Contributor

I have a proposed "good-enough" solution for this at Automattic/newspack-plugin#276. Let me know if you want it in this plugin and I'll open a PR here. I'd definitely prefer to contribute it upstream to this plugin instead of maintaining it as a tweak in Newspack.

@benlk
Copy link
Collaborator Author

benlk commented Sep 16, 2019

@claudiulodro Yes, we'd love to have your PR here.

@claudiulodro
Copy link
Contributor

#62

@benlk
Copy link
Collaborator Author

benlk commented Sep 18, 2019

Thanks! We'll take a look as soon as we can.

@benlk benlk closed this as completed in #62 Mar 2, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants