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

Full Site Editing: Add image filters to the template content output #36026

Merged
merged 4 commits into from
Sep 27, 2019

Conversation

Copons
Copy link
Contributor

@Copons Copons commented Sep 6, 2019

Changes proposed in this Pull Request

  • Add more the_content filter functions to the template content output in order to fix how the manually resized images in the template part look on the front end.
Before After
Screenshot 2019-09-06 at 11 47 28 Screenshot 2019-09-06 at 11 33 46

These are the default the_content filters that we should roughly use in FSE, and here are the prepend_attachment and wp_make_content_images_responsive functions.

Testing instructions

  • Either:
    • Apply D33228-code and sandbox a WPCOM FSE site with Maywood.
    • Or just open a non-WPCOM FSE site with Maywood.
  • Add an image to the header, and resize it using the resize handles (not the size selector in the sidebar).
  • Preview the image: the size should be the same.
  • Repeat the test on the post content, and make sure the image size is the same in both editor and front end.

Fixes #36020

@Copons Copons added [Pri] BLOCKER [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. [Goal] Full Site Editing labels Sep 6, 2019
@Copons Copons requested review from apeatling and a team September 6, 2019 10:53
@Copons Copons requested a review from a team as a code owner September 6, 2019 10:53
@Copons Copons self-assigned this Sep 6, 2019
@matticbot
Copy link
Contributor

@matticbot
Copy link
Contributor

This PR does not affect the size of JS and CSS bundles shipped to the user's browser.

Generated by performance advisor bot at iscalypsofastyet.com.

@Copons
Copy link
Contributor Author

Copons commented Sep 6, 2019

@iamtakashi I've noticed that non explicitly aligned images are always centered in the front end.
Is it the intended behaviour?

@iamtakashi
Copy link
Contributor

@Copons Hmm, it appears left-aligned to me. Do you have a URL where I can see what you're seeing?

Screen Shot 2019-09-06 at 16 06 06

Screen Shot 2019-09-06 at 16 06 25

@Copons
Copy link
Contributor Author

Copons commented Sep 6, 2019

@iamtakashi Sorry, you're right, I forgot to mention "manually resized images".

This is on a non-FSE site using Varia (but the same happens with Maywood):

Sep-06-2019 16-26-39

@iamtakashi
Copy link
Contributor

@Copons Oh.. nice catch. We need to fix this in Varia and child themes.

// 11 priority
$content = do_shortcode( $content );

$content = prepend_attachment( $content );
$content = wp_make_content_images_responsive( $content );
Copy link
Contributor

Choose a reason for hiding this comment

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

Non-blocking, but I think we'll probably want to start tracking these as units in the isolated suite on the wpcom side. It makes it much easier to visualize the post_content transform and catch regressions.

Copy link
Contributor

Choose a reason for hiding this comment

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

Feel free to DM if you need help with test setup.

Copy link
Contributor

@gwwar gwwar left a comment

Choose a reason for hiding this comment

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

Tested this on wpcom, but I think this is about right. Some other behavior to note:

  1. Header images can't be resized past their natural image dimensions
  2. Also ran into the image alignment issue after resizing (centering when it shouldn't in the published view)

Maybe get a +1 for if the filters look correct

@Copons
Copy link
Contributor Author

Copons commented Sep 19, 2019

@gwwar

  1. Also ran into the image alignment issue after resizing (centering when it shouldn't in the published view)

Yup, noted this in an earlier comment: #36026 (comment)
It appears to be a theme issue unrelated to FSE.

Maybe get a +1 for if the filters look correct

I'll wait for a +1 and meanwhile I'll investigate this:

  1. Header images can't be resized past their natural image dimensions

It might be as easy as having max-width: none (instead of 100%) on .is-resized images. 🤔

EDIT: ditto for this: it's a theme thing. I'll ship this and open an issue tagging theme folks too.

@mattwiebe
Copy link
Contributor

This will need changing on the WPCom side as we handle things differently there. The wp_make_content_images_responsive function is removed from the_content filter and new filters are added via WPCOM_Responsive_Images::set_filters.

Probably the easiest thing would just be something like:

if ( defined( 'IS_WPCOM') && IS_WPCOM ) {
  $instance = new WPCOM_Responsive_Images;
  $content = $instance->make_content_images_responsive( $content );
} else {
  $content = wp_make_content_images_responsive( $content );
}

I don't know for sure if that will work, but it should get us on the right track.

@Copons
Copy link
Contributor Author

Copons commented Sep 26, 2019

Thanks @mattwiebe, that's a very good suggestion and... there even was a todo comment a few lines above pointing to it!
I'll give it a try asap.

@Copons
Copy link
Contributor Author

Copons commented Sep 26, 2019

I've added @mattwiebe's suggestion in 8834b9a.

It works on my local environment and also on my sandboxed test site on WPCOM.

Trying it was as simple as just manually copying and pasting these few lines of code into:
wp-content/plugins/full-site-editing-plugin/0.7/full-site-editing/templates/class-wp-template.php
and perform the test steps provided in the PR description.


// 11 priority
$content = do_shortcode( $content );

$content = prepend_attachment( $content );

if ( defined( 'IS_WPCOM' ) && IS_WPCOM && class_exists( '\WPCOM_Responsive_Images' ) ) {
Copy link
Member

Choose a reason for hiding this comment

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

So far we have tried to avoid adding any Dotcom specific code in the FSE plugin. As a non-blocking suggestion: we could add a filter here and modify the content from dotcom side in our mu-plugin code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oooh, very good point!
It's an easy enough change, I'll do it right now!

@Copons
Copy link
Contributor Author

Copons commented Sep 26, 2019

This now requires D33228-code to work on WPCOM.

@noahtallen
Copy link
Member

noahtallen commented Sep 26, 2019

With the filter being in mu plugins, will this PR work for Atomic? (or is this just a workaround for wpcom which isn't the case in Atomic sites?)

@vindl
Copy link
Member

vindl commented Sep 27, 2019

With the filter being in mu plugins, will this PR work for Atomic? (or is this just a workaround for wpcom which isn't the case in Atomic sites?)

Atomic sites should behave as other self-hosted sites, I don't think they have this Dotcom specific code accessible or running (unless it's explicitly ported via wpcomsh).

@Copons Copons added [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. and removed [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Sep 27, 2019
@Copons
Copy link
Contributor Author

Copons commented Sep 27, 2019

@noahtallen what @vindl said. 🙂
I'm confused about this too, because—even if it's not the case here—what would happen on Simple->Atomic sites if the WPCOM filter was very different than the Core one?
(Rethorical question of course 😄)

Anyway, I think this doesn't depend on the WPCOM diff, and it's good to go now!
Thanks y'all for the reviews and discussion.

@Copons Copons merged commit 6f28a37 into master Sep 27, 2019
@Copons Copons deleted the fix/36020-fse-resized-images branch September 27, 2019 09:51
@matticbot matticbot removed the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Sep 27, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

FSE: Header image size is inconsistent
7 participants