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

Layout block support: Append layout styles to post content in REST API requests #35413

Conversation

andrewserong
Copy link
Contributor

Description

Fixes #35376

This PR updates the Layout block support to render the layout styles in REST API requests by appending the styles to the end of post content.

In requests to endpoints like the posts endpoint, wp_footer isn't output, so in certain circumstances (like rendering the Post Content block in the site editor) blocks depending on the layout styles render incorrectly. The approach in this PR attempts to be as consistent as we can with the approach introduced in #32083, which moved the rendering of the styles from the end of the individual block's content to wp_footer.

How has this been tested?

Manually.

Before applying this PR:

  1. Activate a blocks-based theme, e.g. TT1-Blocks
  2. Add a post and insert a Social Icons block, and add a bunch of icons with links
  3. Publish the post and open up the Site Editor
  4. Assuming your theme uses the Query Loop + Post Content blocks in the main index template, you should see the issue reported in Post Content: Layout styles not applied in editor (e.g. Social Icons, Buttons, etc.) #35376 where the layout spacing styles used by the Social Icons block is not rendered within the editor.

After applying this PR:

  1. Reload the site editor, and you should see that the styling issue is resolved
  2. View the front end of your site — the social icons should still render correctly, and if you view source, you should see that the wp-container styles (e.g. <style>.wp-container-615e737c1a8f3 > *) are still rendered in the footer, after script tags, etc.
  3. You can also test the REST API endpoint directly by grabbing the post id of your published post. Then, in the site editor, once you've reloaded the page, search the Network tab of dev tools for your post id. You should find a network request for the posts API endpoint, used to render the Post Content block within the editor. E.g. on my test site, there's a URL like: http://localhost:8888/index.php?rest_route=%2Fwp%2Fv2%2Fposts%2F1105&_locale=user
  4. If you inspect the JSON returned by that endpoint, at the end of the content.rendered string, you should see the layout styles are now rendered

Screenshots

Editor before Editor after
image image
JSON before JSON after
image image

Types of changes

Bug fix (non-breaking change which fixes an issue)

Checklist:

  • My code is tested. (manually)
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • I've tested my changes with keyboard and screen readers.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.
  • I've updated all React Native files affected by any refactorings/renamings in this PR (please manually search all *.native.js files for terms that need renaming or removal).

@andrewserong andrewserong added [Type] Bug An existing feature does not function as intended [Block] Social Affects the Social Block - used to display Social Media accounts [Feature] Full Site Editing [Block] Post Content Affects the Post Content Block labels Oct 7, 2021
@andrewserong andrewserong self-assigned this Oct 7, 2021
@andrewserong
Copy link
Contributor Author

@youknowriad and @ellatrix, I've just added you both as reviewers since you've worked on the layout support before. Please let me know if you think there's another way we should handle this instead 🙂

@youknowriad
Copy link
Contributor

Interesting issue, I agree we need to find a consistent solution here but the proposed one is rather too specific for me. I think there might be other block supports (duotone and maybe others?) that have the same behavior as layout and we should try to find something baked into the framework and avoid moving the responsibility to handle this to the block supports them selves.

@ramonjd
Copy link
Member

ramonjd commented Oct 7, 2021

This works well for me @andrewserong

Thanks for getting a fix in so quickly 🙇

Before After
Screen Shot 2021-10-07 at 6 28 54 pm Screen Shot 2021-10-07 at 6 23 34 pm

✅ Layout block CSS is appended to the post content in the REST API response.
✅ Social Icons blocks appear as they should in the Post Content Block in the Site Editor
✅ Block Editor styles are unaffected (no change) and frontend appears as it should.

It looks good to me, but keen to hear other's opinion on the approach.

@ramonjd
Copy link
Member

ramonjd commented Oct 7, 2021

I think there might be other block supports (duotone and maybe others?) that have the same behavior as layout and we should try to find something baked into the framework and avoid moving the responsibility to handle this to the block supports them selves

That sounds very reasonable 👍

I can confirm that the duotone render block callback is also affected:

Image Block with duotone in Block Editor Post Content Block in Site Editor
Screen Shot 2021-10-07 at 7 22 50 pm Screen Shot 2021-10-07 at 7 22 44 pm

Is there an obvious place to relocate the logic to handle this, or to start looking?

I was thinking, because I don't know any better, that we could roll out a helper method/filter that will do the REST_API check and do the work accordingly.

@youknowriad
Copy link
Contributor

To be honest, I don't know. I think @ellatrix moved these styles to the footer initially so she might know better.

@andrewserong
Copy link
Contributor Author

andrewserong commented Oct 7, 2021

Thanks for the feedback, folks!

I think there might be other block supports (duotone and maybe others?) that have the same behavior as layout and we should try to find something baked into the framework and avoid moving the responsibility to handle this to the block supports them selves.

Very much agreed, it does feel quite brittle if each of the block supports has to worry about how and where they're injecting styles. It'd be nice to have a helper method or two that abstracts the behaviour away.

Let me know if anyone thinks of other ways to approach this, I'm happy to do further digging, too. While I was hacking around with this PR, I tried a few different ideas, including hooking into filtering the REST API response, and ironically the solution using an extra filter and the_content looked much cleaner in comparison 😅

I suppose ultimately we need to come up with a consistent approach for block supports that:

@youknowriad
Copy link
Contributor

That makes sense.

It makes me wonder about global styles styles though, some of the global styles are also necessary for post content to show up properly and sometimes the distinction about what should be in post content or not is hard to draw.

@youknowriad
Copy link
Contributor

Note that after #35425 the link color styles will also be impacted by this bug.

@andrewserong
Copy link
Contributor Author

I'm wrapping up for the week, but out of curiosity, I thought I'd explore the idea of whether the approach in this PR could be generalised and made applicable for the three block supports that currently render either svg or styles and append to the post/page via wp_footer (and therefore need handling for the REST API) — Layout, Duotone, and Elements.

I've made a start with a draft PR in #35450, which currently refactors the Layout support a bit to make this possible. I think the approach can be wrangled to also factor in the Duotone and Elements implementations, but the whole concept could still be a bit of hack. I'm curious to see whether I can get it to technically work, but I'm more than happy to get early feedback if anyone thinks it isn't worth pursuing further. Otherwise, I'll keep hacking around / digging in that draft PR next week! 😀

@youknowriad
Copy link
Contributor

A common function sounds good like explored in #35450 but I didn't understand why you had to include the "placeholder" logic there, it could receive the "output" directly?

@andrewserong
Copy link
Contributor Author

Thanks for taking a look!

I didn't understand why you had to include the "placeholder" logic there, it could receive the "output" directly?

I was curious to explore the idea of seeing if we could abstract away the unique id generation to the function, too, instead of the block supports having to worry about it. But it's probably doing a bit too much to add that in at the same time (and then adds the complexity of the "placeholder" logic). I'll rework it on Monday to simplify. Cheers!

Copy link
Member

@spacedmonkey spacedmonkey left a comment

Choose a reason for hiding this comment

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

Hey, I am maintainer of the REST API, so want to chime here.

First, I don't like the format of this PR, the_content is used for every post type, even hidden ones, like menu items. Using it here, is overkill and will fire on many post types that do not need it. Also, the check for REST API, could be improved, as it will not load in preloading middleware.

Instead of adding to new field the REST API, using a function like register_rest_field. Why does this data need to live in the content field?

If adding a new field doesn't make sense, can I recommend using, rest_prepare_{$this->post_type}.

Let me know, when you have made the changes and I will re-review

@andrewserong
Copy link
Contributor Author

Thanks for taking a look @spacedmonkey, much appreciated!

First, I don't like the format of this PR, the_content is used for every post type, even hidden ones, like menu items. Using it here, is overkill and will fire on many post types that do not need it ... Instead of adding to new field the REST API, using a function like register_rest_field. Why does this data need to live in the content field?

That's a great idea. Preferably we wouldn't be rendering these style elements to the content field (it felt messy to me too, but I couldn't quite think of an alternative while I was hacking around last week). I agree, it'd be much nicer if we had a separate field for it. Since this PR primarily intends to fix how blocks are rendered within the editor, using a separate field in the API response sounds much cleaner.

I'll do some exploration either in this PR or in #35450 to see what's possible and will be sure to give you a ping when it's ready for another review!

@andrewserong
Copy link
Contributor Author

I'm going to close this PR out now, as I've progressed further in moving to a generic utility function approach in #35450, so let's move the discussion over there. Thanks!

@ellatrix ellatrix deleted the update/layout-support-to-append-styles-to-the-content-in-api-requests branch October 12, 2021 06:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Block] Post Content Affects the Post Content Block [Block] Social Affects the Social Block - used to display Social Media accounts [Type] Bug An existing feature does not function as intended
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Post Content: Layout styles not applied in editor (e.g. Social Icons, Buttons, etc.)
4 participants