Add AMP-compatibility to pym shortcodes/blocks/raw embeds #62
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This was referenced Sep 18, 2019
Closed
This will be unnecessary once ampproject/amphtml#22714 is resolved. |
|
How will ampproject/amphtml#22714 resolve the need for sites to edit their Pym child pages? |
Child pages would no longer be required to include this code: <script>window.addEventListener( 'load', () => {
function sendEmbedSize() {
window.parent.postMessage({
sentinel: 'amp',
type: 'embed-size',
height: document.body.scrollHeight
}, '*');
}
sendEmbedSize();
// Try to make sure elements created dynamically (e.g. graphics) are sized properly.
setTimeout( sendEmbedSize, 500 );
// Update embed size after clicks.
document.addEventListener( 'click', sendEmbedSize, { passive: true } );
} );
</script>Because |
|
@westonruter Ah, okay, that makes sense. |
Our version numbering strategy is described at https://github.com/INN/pym-shortcode/blob/master/docs/maintainer-notes.md . Since the current version of Pym.js is 1.3.2, and the last release of this plugin was 1.3.2.2, you should use Is the test case you included in the pull request text the only test you used? If so, can you run through as many of the tests at https://github.com/INN/pym-shortcode/blob/master/docs/maintainer-notes.md#testing-before-release as you have time for? |
This was referenced Sep 20, 2019
Closed
Merged
|
I've updated the I've also tested the test cases you've linked, and added handling for alignment. The test cases all display on AMP. The only issue I noticed is the known resizing issue (there might be too much/too little padding on existing embeds that don't have the resize code snippet until AMP gets native Pym.js compatibility). I believe this should be ready to go, pending your review and approval. :) |
|
Here's a screenshot of a page using several embeds from the unmodified https://blog.apps.npr.org/pym.js/examples/table/child.html : The embeds have initialized in square mode, without a resize button. page content
|
|
Here's similar post content, but using https://f.benlk.com/graphics/heartbeat-abortion-bills-amp/child.html, a version of https://f.benlk.com/graphics/heartbeat-abortion-bills/child.html that has been modified by adding the code included in this PR. Post contentscreenshotUpon page load, the graphics initialize, but are then replaced with an empty white space. Here's an example of a shortcode, and the markup that generates on the page after the page finishes loading: |
|
On the off chance that it's some problem with my example page, I copied the NPR example above to https://f.benlk.com/graphics/child-table/child.html and added the snippet. post contentscreenshot
The same "flash embed, then display blank" behavior occurs, but with a different height of blank area. The height of the blank area appears to match the height of the graphic before it was blanked out. This is occurring for all embeds on the page, not just ones that load above the fold. There are no console errors or messages. After the flash of content, the Sometimes (but not always), scrolling the page as it loads allows one of the graphics on the page to load. @claudiulodro any idea what's going on here? |
|
The above testing was performed using the "Atomic Blocks" theme https://wordpress.org/themes/atomic-blocks/ but reproduces in Twenty Nineteen. |
|
The example post content given in the "testing" section of this post works like the default NPR example: the okwatch embeds load, but in a square frame. https://s3.us-west-2.amazonaws.com/projects.oklahomawatch.org/oklahoma-legislature/senate.html does not not appear to have been modified for this PR. |
|
I'll investigate and follow up. Thanks for testing! On a related note, the Pym.js message handling is coming along nicely: ampproject/amphtml#24917 |
|
FYI: I've added a pull request to implement Pym.js support in |
|
@benlk I wasn't able to fully reproduce the issue you were describing using the sample content you provided and the themes you used. I was, however, able to reproduce it on the alignleft embed. I believe AMP was getting confused somehow because the placeholder element and resize element were the same. 42bab35 solves the issue in my testing; please let me know if it solves the issue in your testing. Uninitialized: Initialized: |
|
@benlk have you had a chance to give this PR another test? The "empty-white-space" issue should be resolved in theory. Thanks! |
|
@claudiulodro I'm sad to say I haven't, and we probably won't be able to test it again before November 1. Does that affect the timeline for the WP AMP plugin or Newspack plugin updates? |
|
Not a huge deal, thanks for checking. On the Newspack side, we can just use this fork for now until it's officially merged. |
|
@benlk just following up again. Have you had a chance to re-test this? It would be nice to get these improvements merged. Thanks. |
|
We're taking another look at this today! |
benlk
reviewed
Nov 22, 2019
|
I'm okay with merging this, because I think the problems I'm seeing are to do with the interactives I'm using to test, not with the plugin's implementation. But we'd like to be sure. All of this comment's tests are using the twentynineteen theme on WP 5.3. The basic problem is that Pym embeds loaded via AMP aren't taking up the full width that the theme gives to the block. The following example uses https://f.benlk.com/graphics/child-table/child.html, which has the added event listener. screenshot, code
<!-- wp:paragraph -->
<p>The following is a Pym Embed adapted in accordance with the instructions at https://github.com/INN/pym-shortcode/pull/62:</p>
<!-- /wp:paragraph -->
<!-- wp:paragraph -->
<p>(Full width)</p>
<!-- /wp:paragraph -->
<!-- wp:pym-shortcode/pym {"src":"https://f.benlk.com/graphics/child-table/child.html","align":"full"} -->
<a href="https://f.benlk.com/graphics/child-table/child.html" class="wp-block-pym-shortcode-pym alignfull">https://f.benlk.com/graphics/child-table/child.html</a>
<!-- /wp:pym-shortcode/pym -->
<!-- wp:paragraph -->
<p>Lorem ipsum dolor sit amet, consectetur adipiscing elit. Donec finibus ligula id est volutpat suscipit. Etiam tempus turpis vel arcu ultricies porttitor. Cras vel bibendum dui, in porttitor ipsum. Phasellus nec nisi eros. Morbi mollis sapien nec vulputate dignissim. Proin justo tortor, ullamcorper non porttitor quis, eleifend non nisi. </p>
<!-- /wp:paragraph -->
<!-- wp:paragraph -->
<p>(wide width)</p>
<!-- /wp:paragraph -->
<!-- wp:pym-shortcode/pym {"src":"https://f.benlk.com/graphics/child-table/child.html","align":"wide"} -->
<a href="https://f.benlk.com/graphics/child-table/child.html" class="wp-block-pym-shortcode-pym alignwide">https://f.benlk.com/graphics/child-table/child.html</a>
<!-- /wp:pym-shortcode/pym -->
<!-- wp:paragraph -->
<p>Nam faucibus ornare elit, ut ultrices nibh aliquet a. Donec varius, lorem molestie fringilla eleifend, mauris nisl porta tellus, non varius leo urna eu elit. Orci varius natoque penatibus et magnis dis parturient montes, nascetur ridiculus mus. Orci varius natoque penatibus et magnis dis parturient montes, nascetur ridiculus mus. Suspendisse porttitor varius ex sed facilisis. Donec lobortis mauris risus, at dictum sapien ullamcorper a. Quisque tempus eu ipsum at tincidunt. Donec sit amet faucibus nisi.</p>
<!-- /wp:paragraph -->
<!-- wp:paragraph -->
<p>(normal width)</p>
<!-- /wp:paragraph -->
<!-- wp:paragraph -->
<p><code>pym src="https://f.benlk.com/graphics/child-table/child.html" class="one two three four"</code></p>
<!-- /wp:paragraph -->
<!-- wp:shortcode -->
[pym src="https://f.benlk.com/graphics/child-table/child.html" class="one two three four"]
<!-- /wp:shortcode -->
<!-- wp:paragraph -->
<p>Maecenas facilisis dignissim turpis, eu rutrum purus aliquet ac. Sed pellentesque aliquet enim, eget sodales augue elementum a. In hac habitasse platea dictumst. Etiam sit amet massa porttitor, auctor nisi tristique, interdum sem. Mauris eget tincidunt eros. Etiam a ullamcorper nulla. Nunc sed enim varius, hendrerit nulla rhoncus, convallis arcu. Sed ac mattis ligula, nec feugiat nisl. Donec eu sodales neque. Proin rhoncus sagittis mi sed eleifend. </p>
<!-- /wp:paragraph -->
<!-- wp:paragraph -->
<p>The following is the same embed, but as a Shortcode Block:</p>
<!-- /wp:paragraph -->
<!-- wp:paragraph -->
<p><code>pym src="https://f.benlk.com/graphics/child-table/child.html" align="left" id="extremely_specific_id" class="one two three four"</code></p>
<!-- /wp:paragraph -->
<!-- wp:shortcode -->
[pym src="https://f.benlk.com/graphics/child-table/child.html" align="right" id="extremely_specific_id" class="one two three four"]
<!-- /wp:shortcode -->
<!-- wp:paragraph -->
<p>Aenean quis diam maximus, molestie diam eget, consequat nisl. Nunc dignissim tortor non turpis elementum, id iaculis mi consectetur. In eget odio ut eros maximus lobortis sed eu ante. Cras dictum urna et lobortis sagittis. Quisque rhoncus blandit maximus. Sed porttitor efficitur mi, ut mattis justo consequat aliquet.</p>
<!-- /wp:paragraph -->
<!-- wp:paragraph -->
<p>Sed ante nibh, dictum pretium iaculis nec, lobortis ut magna. Duis lobortis pulvinar metus, eget vulputate justo molestie ac. Ut gravida risus arcu, sed sollicitudin erat laoreet eu. Aliquam efficitur justo vel sapien commodo, eget ultrices nisl ullamcorper. Donec id dui a ligula interdum ultricies. Sed sit amet odio velit. Aliquam erat volutpat. Donec varius massa ac nibh blandit imperdiet. Suspendisse vestibulum, lectus vitae suscipit tincidunt, odio nulla efficitur urna, nec molestie mauris lacus posuere ante. In molestie sit amet dolor ac auctor. Ut lectus lectus, mollis vel consectetur nec, imperdiet id ex. Suspendisse at arcu eleifend, pharetra nisi ac, tempus magna. Pellentesque venenatis odio non vestibulum accumsan. Suspendisse potenti. Aliquam a massa libero.</p>
<!-- /wp:paragraph -->
<!-- wp:paragraph -->
<p>Sed ante nibh, dictum pretium iaculis nec, lobortis ut magna. Duis lobortis pulvinar metus, eget vulputate justo molestie ac. Ut gravida risus arcu, sed sollicitudin erat laoreet eu. Aliquam efficitur justo vel sapien commodo, eget ultrices nisl ullamcorper. Donec id dui a ligula interdum ultricies. Sed sit amet odio velit. Aliquam erat volutpat. Donec varius massa ac nibh blandit imperdiet. Suspendisse vestibulum, lectus vitae suscipit tincidunt, odio nulla efficitur urna, nec molestie mauris lacus posuere ante. In molestie sit amet dolor ac auctor. Ut lectus lectus, mollis vel consectetur nec, imperdiet id ex. Suspendisse at arcu eleifend, pharetra nisi ac, tempus magna. Pellentesque venenatis odio non vestibulum accumsan. Suspendisse potenti. Aliquam a massa libero.</p>
<!-- /wp:paragraph -->
<!-- wp:paragraph -->
<p>Sed iaculis placerat aliquam. Phasellus risus risus, eleifend dictum luctus nec, iaculis vel augue. In id purus a sem finibus aliquet aliquam a velit. Cras elit ante, lobortis eu commodo sit amet, sollicitudin sit amet mi. Proin ornare mauris nec iaculis aliquet. In hac habitasse platea dictumst. In gravida leo ut feugiat maximus. Suspendisse ac libero bibendum, dapibus diam a, posuere magna.</p>
<!-- /wp:paragraph -->
<!-- wp:paragraph -->
<p>In eu malesuada elit. Sed quis ex gravida, tempus eros eget, porta massa. Pellentesque quis mollis eros. Vestibulum et vehicula urna. Pellentesque dui libero, congue ut lacus quis, bibendum sagittis risus. Vestibulum hendrerit ultrices arcu, eget mattis tortor dapibus et. Ut condimentum finibus eros a ultrices. Nunc at rhoncus velit.</p>
<!-- /wp:paragraph -->Note that:
Here's a replication using https://f.benlk.com/graphics/heartbeat-abortion-bills-amp/child.html which also has the event listener. screenshot, code
<!-- wp:paragraph -->
<p>The following is a Pym Embed adapted in accordance with the instructions at https://github.com/INN/pym-shortcode/pull/62:</p>
<!-- /wp:paragraph -->
<!-- wp:pym-shortcode/pym {"src":"https://f.benlk.com/graphics/heartbeat-abortion-bills-amp/child.html","align":"full"} -->
<a href="https://f.benlk.com/graphics/heartbeat-abortion-bills-amp/child.html" class="wp-block-pym-shortcode-pym alignfull">https://f.benlk.com/graphics/heartbeat-abortion-bills-amp/child.html</a>
<!-- /wp:pym-shortcode/pym -->
<!-- wp:paragraph -->
<p>Lorem ipsum dolor sit amet, consectetur adipiscing elit. Donec finibus ligula id est volutpat suscipit. Etiam tempus turpis vel arcu ultricies porttitor. Cras vel bibendum dui, in porttitor ipsum. Phasellus nec nisi eros. Morbi mollis sapien nec vulputate dignissim. Proin justo tortor, ullamcorper non porttitor quis, eleifend non nisi. </p>
<!-- /wp:paragraph -->
<!-- wp:paragraph -->
<p>Shortcode: <code>pym src="https://f.benlk.com/graphics/heartbeat-abortion-bills-amp/child.html"</code></p>
<!-- /wp:paragraph -->
<!-- wp:shortcode -->
[pym src="https://f.benlk.com/graphics/heartbeat-abortion-bills-amp/child.html"]
<!-- /wp:shortcode -->
<!-- wp:paragraph -->
<p>Nam faucibus ornare elit, ut ultrices nibh aliquet a. Donec varius, lorem molestie fringilla eleifend, mauris nisl porta tellus, non varius leo urna eu elit. Orci varius natoque penatibus et magnis dis parturient montes, nascetur ridiculus mus. Orci varius natoque penatibus et magnis dis parturient montes, nascetur ridiculus mus. Suspendisse porttitor varius ex sed facilisis. Donec lobortis mauris risus, at dictum sapien ullamcorper a. Quisque tempus eu ipsum at tincidunt. Donec sit amet faucibus nisi.</p>
<!-- /wp:paragraph -->
<!-- wp:quote {"align":"left"} -->
<blockquote class="wp-block-quote has-text-align-left"><p> “It’s a shame, but what can we do? There really wasn’t anything that was going to keep this individual from snapping and killing a lot of people if that’s what they really wanted.”</p></blockquote>
<!-- /wp:quote -->
<!-- wp:paragraph -->
<p>Maecenas facilisis dignissim turpis, eu rutrum purus aliquet ac. Sed pellentesque aliquet enim, eget sodales augue elementum a. In hac habitasse platea dictumst. Etiam sit amet massa porttitor, auctor nisi tristique, interdum sem. Mauris eget tincidunt eros. Etiam a ullamcorper nulla. Nunc sed enim varius, hendrerit nulla rhoncus, convallis arcu. Sed ac mattis ligula, nec feugiat nisl. Donec eu sodales neque. Proin rhoncus sagittis mi sed eleifend. </p>
<!-- /wp:paragraph -->
<!-- wp:paragraph -->
<p><code>pym src="https://f.benlk.com/graphics/heartbeat-abortion-bills-amp/child.html" align="left" id="extremely_specific_id" class="one two three four"</code></p>
<!-- /wp:paragraph -->
<!-- wp:shortcode -->
[pym src="https://f.benlk.com/graphics/heartbeat-abortion-bills-amp/child.html" align="left" id="extremely_specific_id" class="one two three four"]
<!-- /wp:shortcode -->
<!-- wp:paragraph -->
<p>Aenean quis diam maximus, molestie diam eget, consequat nisl. Nunc dignissim tortor non turpis elementum, id iaculis mi consectetur. In eget odio ut eros maximus lobortis sed eu ante. Cras dictum urna et lobortis sagittis. Quisque rhoncus blandit maximus. Sed porttitor efficitur mi, ut mattis justo consequat aliquet.</p>
<!-- /wp:paragraph -->
<!-- wp:paragraph -->
<p>The following is the same embed, but as a Shortcode Block:</p>
<!-- /wp:paragraph -->
<!-- wp:paragraph -->
<p>Sed ante nibh, dictum pretium iaculis nec, lobortis ut magna. Duis lobortis pulvinar metus, eget vulputate justo molestie ac. Ut gravida risus arcu, sed sollicitudin erat laoreet eu. Aliquam efficitur justo vel sapien commodo, eget ultrices nisl ullamcorper. Donec id dui a ligula interdum ultricies. Sed sit amet odio velit. Aliquam erat volutpat. Donec varius massa ac nibh blandit imperdiet. Suspendisse vestibulum, lectus vitae suscipit tincidunt, odio nulla efficitur urna, nec molestie mauris lacus posuere ante. In molestie sit amet dolor ac auctor. Ut lectus lectus, mollis vel consectetur nec, imperdiet id ex. Suspendisse at arcu eleifend, pharetra nisi ac, tempus magna. Pellentesque venenatis odio non vestibulum accumsan. Suspendisse potenti. Aliquam a massa libero.</p>
<!-- /wp:paragraph -->
<!-- wp:paragraph -->
<p>Sed iaculis placerat aliquam. Phasellus risus risus, eleifend dictum luctus nec, iaculis vel augue. In id purus a sem finibus aliquet aliquam a velit. Cras elit ante, lobortis eu commodo sit amet, sollicitudin sit amet mi. Proin ornare mauris nec iaculis aliquet. In hac habitasse platea dictumst. In gravida leo ut feugiat maximus. Suspendisse ac libero bibendum, dapibus diam a, posuere magna.</p>
<!-- /wp:paragraph -->
<!-- wp:paragraph -->
<p>In eu malesuada elit. Sed quis ex gravida, tempus eros eget, porta massa. Pellentesque quis mollis eros. Vestibulum et vehicula urna. Pellentesque dui libero, congue ut lacus quis, bibendum sagittis risus. Vestibulum hendrerit ultrices arcu, eget mattis tortor dapibus et. Ut condimentum finibus eros a ultrices. Nunc at rhoncus velit.</p>
<!-- /wp:paragraph -->Here, the full-width and wide-width blocks aren't being rendered at full width. The third graphic, aligned left, hadn't loaded yet because I hadn't scrolled down that far before telling Firefox to screenshot the page. In later reloads of the page, the graphic was the width it normally is when aligned left. If you can provide us with links for the embeds that you've been using for testing this branch, and they work, their successful loading will rule out the problem being in this branch of the plugin. We'd be happy to merge this. If there's any other information you need, let us know. |
|
Thanks for giving it another test! I haven't been able to reproduce the issues you describe using either Chrome or Firefox 70, 2019 theme, WP 5.3, and the embeds you've been testing with: Screenshot 1
Screenshot 2
I'm not sure how you want to proceed given this. I can wrangle another person or two if you want to cross-reference? IMO any AMP-compatibility is going to be better than the current no-AMP-compatibility, so it might be best to merge and then fine-tune as we get more data points from real sites. |
westonruter
reviewed
Jan 29, 2020
|
FYI: Pym.js support in |
benlk
approved these changes
Mar 2, 2020
There was a problem hiding this comment.
With the amp plugin at 1.4.4, approved with the modifications made in https://github.com/INN/pym-shortcode/pull/62/files/42bab35107f22e320a557adc2112421ee97d5e39#r372559828
benlk
added a commit
that referenced
this pull request
Mar 2, 2020
Implements change suggested by @westonruter in #62 (comment)
43 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Changes
If the snippet is not added, the embed will be rendered in a square aspect ratio but will otherwise continue working correctly. When AMP gets native Pym-compatibility, this step will ideally not be required any more. The snippet shouldn't affect anything on non-AMP pages.
Why
Closes #59
Testing/Questions
Questions that need to be answered before merging:
@sincein the new functions?Steps to test this PR:
To test resizing:
3. Clicking on that will resize the height of the graphic correctly: