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.
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
Post Content Block: Render readonly content as blocks to preserve block supports styles #35863
Post Content Block: Render readonly content as blocks to preserve block supports styles #35863
Changes from 7 commits
cd78f31
9a680bd
4a296d5
6c03989
04caf9b
b06b374
2a5a246
284ded1
89fba21
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since now both
EditableContent
andReadOnlyContent
use thelayout
we can remove this logic from both and add it only toContent
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, I've moved the layout logic up to the Content component 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm little hesitant with this change from
content.rendered
tocontent.raw
. I think that this might lead to security issues with private content.. @peterwilsoncc can you share your thoughts?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's correct: previews need to use the rendered version as the raw version may not be available to all users. For example authors don't have raw rights for others' posts, contributors for any published post.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for raising this @ntsekouras and for the feedback @peterwilsoncc! In the use case in this PR, unfortunately I don't think it'll work to use
content.rendered
as we need the raw markup of blocks in order to be able to parse them and have the styles be rendered properly by the preview.As a bit of background, this PR is an alternative / workaround for the issue of rendering block supports styles on the server in order to display styles correctly in the editor. We currently have no way of retrieving the block supports styles from the server via an API request, so the next best option in this PR was to allow the editor to generate the styles from the block markup.
In the case of the Post Content block, in most cases, I imagine that users will see this block in the editor when editing template parts rather than in an isolated post. Because author and contributor roles don't have access to the site editor, in practice, I'm wondering if it isn't too much of an issue if we have the Post Content preview depend on access to
content.raw
. We already only render the blocks ifcontent.raw
is available, otherwise this falls back to rendering an empty preview, so on balance I think it could be more beneficial to prioritise the correct rendering for the admin view of the block, rather than focusing on author/contributor roles being able to view it in the editor.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the logic for the layout something that could be moved to
useBlockPreview
? You could potentially read thelayout
attribute from the block edit context.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In theory,
useBlockProps
can be called insideuseBlockPreview
, too. We might need the same hook for Comment Content block one day so I'm thinking about how to make this hook easier to use in other places.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a neat idea — I think for the moment, I'm leaning toward keeping this particular layout logic outside of the
useBlockPreview
hook, in case it's possible for us to use the hook in other circumstances whereuseBlockProps
isn't appropriate. I'm thinking of arbitrary block previews that are used elsewhere in the repo, for example in the block switcher or for block patterns. They currently use theBlockPreview
component directly, but it could be good to retain some flexibility for the hook in case we wanted to roll it out there, (or internally in theBlockPreview
component itself?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are absolutely right. I didn't think about the original use case of
<BlockPreview />
that was hidden in the changes for this PR. I guess the usage of the preview inside theedit
is rather an edge case so we shouldn't expect that the block edit context is set here 👍