Skip to content
This repository was archived by the owner on Aug 18, 2026. It is now read-only.

Pym.js AMP-compatibility - #276

Merged
claudiulodro merged 6 commits into
masterfrom
add/pymjsamp
Mar 9, 2020
Merged

Pym.js AMP-compatibility#276
claudiulodro merged 6 commits into
masterfrom
add/pymjsamp

Conversation

@claudiulodro

@claudiulodro claudiulodro commented Sep 14, 2019

Copy link
Copy Markdown
Contributor

All Submissions:

Changes proposed in this Pull Request:

This PR adds basic AMP-compatibility to Pym.js embeds. This is a "good enough" solution until native pym.js AMP compatibility. In general, embeds are designed rectangular, so there will be a little extra padding underneath the amp-iframe, as this renders a square container. We don't have access to the intended height of the iframe from only the src url, and we don't know the exact rectangle ratio so square is a safe choice.

An alternate approach I considered that would always "work correctly" is to detect whether a page is using Pym.js embeds, and if the page is using Pym.js embeds force the page to render in non-AMP. That is also not super ideal, but let me know if you think that's a better solution.

How to test the changes in this Pull Request:

  1. Install and activate Pym.js plugin through the Newspack area of the Plugins screen.
  2. Add the following raw content (in details) to a page:
Details 
<!-- wp:paragraph -->
<p><a title="Atque." href="http://hills.com/soluta-corrupti-inventore-in-nulla-nisi-fuga">Minima assumenda sed est natus ut.</a> quidem optio neque eum Odio sed sapiente ut recusandae molestiae. Sed laboriosam quas veniam. Error soluta vel Est eum dicta nesciunt quaerat saepe. Quaerat aut et officia Esse deserunt voluptate ea possimus Nihil quia ut doloribus dicta repudiandae. Dolorem soluta perferendis odit. Quia ut consequuntur eum ea harum. Quia expedita voluptates repellat omnis est. Corporis cum adipisci ut. Laboriosam voluptatibus eligendi recusandae ducimus molestias. voluptas adipisci ipsam. Corporis voluptatibus unde facilis repellendus quia neque <a title="Expedita ipsam alias nisi tenetur dolorem." href="http://www.hahn.com/nemo-tenetur-debitis-porro-aut-perferendis-esse-facere">Impedit illo non dignissimos nemo</a></p>
<!-- /wp:paragraph -->

<!-- wp:html -->
<div id="senate" data-pym-src="https://s3.us-west-2.amazonaws.com/projects.oklahomawatch.org/oklahoma-legislature/senate.html"></div>
<!-- /wp:html -->

<!-- wp:paragraph -->
<p>Quasi nihil in assumenda qui amet eum Et ut qui iusto maiores. Rem sint unde quasi. Earum pariatur sunt sed consequatur <a href="http://luettgen.com/enim-molestias-hic-officiis-incidunt-corporis-vel">Expedita</a> omnis harum <a href="http://fay.com/dolor-libero-enim-ducimus-ut.html">dolores. Blanditiis</a> temporibus rerum debitis quia ipsam Quaerat distinctio est voluptatem aut expedita. <a href="http://hane.info/">rerum ut numquam laborum</a> Aut temporibus harum Sed suscipit ut nam. Excepturi aut assumenda accusantium voluptates. Veniam amet est optio harum et Ut voluptatem inventore enim velit itaque. qui delectus deleniti corporis. <a href="http://price.biz/dolor-dolor-hic-vel-ea-unde-architecto-nesciunt.html">molestiae</a> sed non. Et voluptate sunt dicta sit voluptas deserunt in. beatae natus quos ad. Necessitatibus iste molestiae harum <a href="https://anderson.org/eum-ad-minima-error-numquam-alias.html">Eaque et eum et nulla. Est est ab</a> magnam. Eos eaque eligendi ut illum molestias. Omnis totam quisquam temporibus sit sit. Cum consequatur omnis nobis officia.</p>
<!-- /wp:paragraph -->

<!-- wp:pym-shortcode/pym {"src":"https://s3.us-west-2.amazonaws.com/projects.oklahomawatch.org/oklahoma-legislature/senate.html"} -->
<a href="https://s3.us-west-2.amazonaws.com/projects.oklahomawatch.org/oklahoma-legislature/senate.html" class="wp-block-pym-shortcode-pym">https://s3.us-west-2.amazonaws.com/projects.oklahomawatch.org/oklahoma-legislature/senate.html</a>
<!-- /wp:pym-shortcode/pym -->

<!-- wp:paragraph -->
<p>Quasi nihil in assumenda qui amet eum Et ut qui iusto maiores. Rem sint unde quasi. Earum pariatur sunt sed consequatur <a href="http://luettgen.com/enim-molestias-hic-officiis-incidunt-corporis-vel">Expedita</a> omnis harum <a href="http://fay.com/dolor-libero-enim-ducimus-ut.html">dolores. Blanditiis</a> temporibus rerum debitis quia ipsam Quaerat distinctio est voluptatem aut expedita. <a href="http://hane.info/">rerum ut numquam laborum</a> Aut temporibus harum Sed suscipit ut nam. Excepturi aut assumenda accusantium voluptates. Veniam amet est optio harum et Ut voluptatem inventore enim velit itaque. qui delectus deleniti corporis. <a href="http://price.biz/dolor-dolor-hic-vel-ea-unde-architecto-nesciunt.html">molestiae</a> sed non. Et voluptate sunt dicta sit voluptas deserunt in. beatae natus quos ad. Necessitatibus iste molestiae harum <a href="https://anderson.org/eum-ad-minima-error-numquam-alias.html">Eaque et eum et nulla. Est est ab</a> magnam. Eos eaque eligendi ut illum molestias. Omnis totam quisquam temporibus sit sit. Cum consequatur omnis nobis officia.</p>
<!-- /wp:paragraph -->

<!-- wp:shortcode -->
[pym src='https://s3.us-west-2.amazonaws.com/projects.oklahomawatch.org/oklahoma-legislature/senate.html']
<!-- /wp:shortcode -->
3. View on the frontend in AMP and regular mode. You should see an interactive map each place there is a Pym.js embed.

.

Note: If you're getting errors because of double-defined newspack_is_amp, check out this branch of the theme.

Other information:

  • Have you added an explanation of what your changes do and why you'd like us to include them?
  • Have you written new tests for your changes, as applicable?
  • Have you successfully ran tests with your changes locally?

@claudiulodro claudiulodro added the [Status] Needs Review The issue or pull request needs to be reviewed label Sep 14, 2019
@claudiulodro claudiulodro mentioned this pull request Sep 14, 2019
Closed
4 tasks
@claudiulodro

claudiulodro commented Sep 14, 2019

Copy link
Copy Markdown
Contributor Author

Offered this PR upstream to the pym.js plugin: Automattic/pym-shortcode#59 (comment)

@jeffersonrabb
jeffersonrabb self-requested a review September 14, 2019 15:33
@claudiulodro

claudiulodro commented Sep 14, 2019

Copy link
Copy Markdown
Contributor Author

@westonruter if you have a chance, I'd appreciate any thoughts on this approach. It's pretty straightforward. I'd also be curious if you know a good solution to making amp-iframe height equal to the height of the inner iframe's content; something like layout='fit' would be cool (good feature idea for the next version of AMP! :D).

westonruter
westonruter reviewed Sep 14, 2019
View reviewed changes
Comment thread includes/class-amp-enhancements.php Outdated
@westonruter

westonruter commented Sep 14, 2019

Copy link
Copy Markdown

I'd also be curious if you know a good solution to making amp-iframe height equal to the height of the inner iframe's content; something like layout='fit' would be cool (good feature idea for the next version of AMP! :D).

Yes, that is supported… but it depends on the document in the iframe window sending this message:

window.parent.postMessage({
  sentinel: 'amp',
  type: 'embed-size',
  height: document.body.scrollHeight
}, '*');

Without the embedded window sending such a message, there is no way for the host page to “see” into the window to determine its dimensions. This is why Pym.js exists, actually.

For more, see the documentation on amp-iframe resizing.

@westonruter

westonruter commented Sep 14, 2019

Copy link
Copy Markdown

It may be possible for you to inject that logic yourself if you have the interactives on the local filesystem:

https://gist.github.com/westonruter/73412f3e35cdeaade808f3b8da9d9586#file-pymjs-amp-injected-js
https://gist.github.com/westonruter/73412f3e35cdeaade808f3b8da9d9586#file-pymjs-testing-php-L96-L119

If the file is located on a remote server requiring HTTP request to fetch, then this would not be a good approach, because the interactive would need to be cached in a transient. When embedding an interactive for things like election results, such added latency is not ideal 😄

@claudiulodro

claudiulodro commented Sep 15, 2019

Copy link
Copy Markdown
Contributor Author

Thanks for the feedback @westonruter! I've implemented the changes you requested and verified the resizing works well on pages that include your above JS snippet.

@jeffersonrabb ping me for instructions if you want to test the resizing.

@claudiulodro claudiulodro added Do Not Merge! and removed [Status] Needs Review The issue or pull request needs to be reviewed labels Sep 16, 2019
@claudiulodro

claudiulodro commented Sep 16, 2019

Copy link
Copy Markdown
Contributor Author

Don't merge this; I'm going to move the functionality to the Pym.js plugin. Automattic/pym-shortcode#59 (comment)

@claudiulodro claudiulodro mentioned this pull request Sep 18, 2019
Merged
@claudiulodro

claudiulodro commented Sep 18, 2019

Copy link
Copy Markdown
Contributor Author

Automattic/pym-shortcode#62

@benlk

benlk commented Mar 2, 2020

Copy link
Copy Markdown

We've merged Automattic/pym-shortcode#62 and are proceeding to get that released to wp.org.

@benlk

benlk commented Mar 3, 2020

Copy link
Copy Markdown

Released: https://github.com/INN/pym-shortcode/releases/tag/v1.3.2.3

@claudiulodro

claudiulodro commented Mar 5, 2020

Copy link
Copy Markdown
Contributor Author

Thanks for getting that AMP-compatibility work released @benlk!

I've removed the handling from here since it's merged upstream, and this PR now just adds Pym.js Embeds to the list of supported plugins.

@claudiulodro claudiulodro added [Status] Needs Review The issue or pull request needs to be reviewed and removed Do Not Merge! labels Mar 5, 2020
@jeffersonrabb jeffersonrabb added [Status] Approved The pull request has been reviewed and is ready to merge and removed [Status] Needs Review The issue or pull request needs to be reviewed labels Mar 9, 2020
@claudiulodro
claudiulodro merged commit a082f32 into master Mar 9, 2020
@claudiulodro
claudiulodro deleted the add/pymjsamp branch March 9, 2020 17:16
benlk
benlk reviewed Mar 9, 2020
View reviewed changes
Comment thread includes/class-plugin-manager.php
@matticbot

matticbot commented Mar 10, 2020

Copy link
Copy Markdown
Contributor

🎉 This PR is included in version 1.1.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

@benlk benlk mentioned this pull request Mar 10, 2020
Open
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

3 more reviewers
@westonruter westonruter westonruter left review comments
@benlk benlk benlk left review comments
@jeffersonrabb jeffersonrabb jeffersonrabb approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

released [Status] Approved The pull request has been reviewed and is ready to merge

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@claudiulodro @westonruter @benlk @matticbot @jeffersonrabb