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

Address issues from testing with WordPress 5.0 #1627

Merged
merged 7 commits into from
Nov 20, 2018
Merged

Conversation

kienstra
Copy link
Contributor

This addresses issues from #1604

When <img> tags have an 'aligncenter' class,
AMP_Img_Sanitizer::handle_centering() wraps theme in
<figure class=aligncenter>.
This ensures that the image inside it is centered.
The AMP runtime sets an inline style on an <amp-img> based
on the sizes attribute if it's present.
For example, <amp-img style="width:calc(50vw)">.
Removing the 'sizes' attribute isn't ideal, but it looks
like it's not possible to override that inline style.
This is only a workaround until the issue is addressed
in the amphtml repo.
If the 'gallery' shortcode has a 'size' attribute of 'thumbnail',
prevent outputting an <amp-carousel>.
That will often get thumbnail images around 150 x 150,
while the <amp-carousel> usually has a width of 600 and a height of 480.
That often means very low-resolution images.
So fall back to the normal 'gallery' shortcode callback, gallery_shortcode().
There's a style rule that prevents these from compressing.
img { object-fit: cover; }
This selector seems to be changed to amp-img in style sanitization,
but it doesn't have the same effect as when applied to img
So copy the style rule verbatim into a stylesheet.
This is in AMP_Gallery_Embed_Handler,
as it's too late to enqueue_style,
'wp_print_styles' has already run by the time
that sanitizer runs.
@kienstra
Copy link
Contributor Author

kienstra commented Nov 20, 2018

Request For Review

Hi @westonruter,
Could you please review this PR, which addresses issues in #1604 (comment)?

Below are the points in that issue:

Image Alignment

  1. There's a diff in image alignment on page. Amp version has:

93a3967 addresses this. To see the issue, see this URL

Gallery Block

  1. Diff in image rendering/resolution. Amp looks like this:

c9e0f03 addresses this. To see this issue, go to this URL. And to test it locally, create a Gallery block like that.

amp-carousel

  1. These images on the Non-AMP site:

It's expected to have those images display as an <amp-carousel>, as they're in a Gallery block with 'Display as AMP carousel' selected.

Twenty Nineteen Image

  1. Blue area appearing at https://paired-ampconfdemo.pantheonsite.io/author/corinne-purtill/?amp. AMP looks like this:

d7de7d1 addresses this.

Follow Us

  1. "Follow Us" widget is empty on non-AMP pages:

There are AMP components, so they're not expected on non-AMP URLs.

amp-carousel

  1. Media Gallery Widget photos are poor quality in AMP version:
    This is a good catch, and something I should have looked at earlier.

Sometimes, there are <amp-carousel> elements with very poor image resolution, like in the Test Media Gallery Widget on this URL.

The media gallery widget often has thumbnail-size images. And the <amp-carousel> usually has a width of 600 and a height of 480. So it's not really intended to produce a large <amp-carousel>, and users might be surprised by how it looks different in AMP.

So 1afa46b prevents outputting an <amp-carousel> if the gallery shortcode (or widget) has a size attribute of thumbnail.

Thanks, Weston!

@kienstra kienstra changed the title [WIP] Address issues from testing with WordPress 5.0 Address issues from testing with WordPress 5.0 Nov 20, 2018
As Weston has mentioned,
there's no need to have any more specificity than this,
like 5.0-beta5
includes/embeds/class-amp-gallery-embed.php Outdated Show resolved Hide resolved
* @return void
*/
public function enqueue_styles() {
wp_enqueue_style(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not a big deal, but why have the external CSS file here but not in add_twentynineteen_image_styles? I'm fine with having such a small amount of CSS just being inline in the PHP.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good question.

This issue with Gallery block styling exists in all of 4 of the Twenty themes this plugin supports.

What do you think of printing the style rule on wp_print_styles:

add_action( 'wp_print_styles', function() {
	?>
	<style>
		.wp-block-gallery.is-cropped .blocks-gallery-item amp-img > img {
			object-fit: cover;
		}
	</style>
	<?php
} );

That works locally.

(Shown as an anonymous function just for this example)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks fine. And using an anonymous function works well for me.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, how does 6b7d305 look?

westonruter and others added 2 commits November 20, 2018 12:03
Apply Weston's suggestion to change add_filter() to add_action()

Co-Authored-By: kienstra <kienstraryan@gmail.com>
As Weston mentioned, it's not necessary to enqueue a stylesheet
that only has one rule.
So output it in a callback for 'wp_print_styles'
@westonruter westonruter added this to the v1.0 milestone Nov 20, 2018
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 this pull request may close these issues.

None yet

2 participants