Add Paragraph prompt to Post Content when empty #50623
Conversation
tellthemachines
added
[Block] Post Content
tellthemachines
requested review from
noisysocks,
jameskoster,
SaxonF,
andrewserong and
ramonjd
|
Size Change: +1.01 kB (0%) Total Size: 1.4 MB
ℹ️ View Unchanged
|
There was a problem hiding this comment.
This is testing nicely for me!
✅ Placeholder text displays when it should, and the Post Content block is visually non-empty in the editor
✅ When editing a page template, the longer-form preview state of the Post Content block is used
Just left a question about optional chaining and whether the wording needs to be more generic for other post types, but other than those two things, this LGTM! ✨
| [ | ||
| 'core/paragraph', | ||
| { | ||
| placeholder: __( 'Type / to add blocks to your page' ), |
There was a problem hiding this comment.
The post content block might not always be in a page. E.g. if we one day have a Posts screen in browse mode. Would it be worth making this more generic?
There was a problem hiding this comment.
Good point, might be best to remove this placeholder so the default one appears.
| @@ -45,12 +46,29 @@ function EditableContent( { context = {} } ) { | |||
| { id: postId } | |||
| ); | |||
|
|
|||
| const hasInnerBlocks = useSelect( | |||
| ( select ) => | |||
| select( blockEditorStore ).getBlock( clientId ).innerBlocks.length > | |||
There was a problem hiding this comment.
getBlock can technically return null. Would it be worth adding optional chaining? This might be a bit of a theoretical issue, and I'm possibly overly cautious when it comes to what these selectors return! 😄
There was a problem hiding this comment.
Always best be cautious!
|
This feels like a good step forward. I know it's close to the hearts of @SaxonF and @jameskoster. GIF:
One issue here is the blue border around the focused paragraph. I know there's a separate effort to clean all that up, but i this case it's extra problematic because it hides the blinking caret, so it's impossible to know you can type. In the post editor we have the concept of "is-writing", where all UI fades away. Do we have something similar here? Essentially if I can type and have a blinking caret, there shouldn't be that selection style. Another option is to just do something like this: ☝️ Very hacky, best to avoid. But it should work for testing. What do you think? |
|
Iirc A unique treatment that removes the border on paragraphs when they are:
Seems okay to me? |
This happens with all text-based blocks in the site editor: Paragraph, Heading, List, Verse, Quote... in all of these cases the caret is hidden. It might be best to address the problem for these blocks as a whole. |
|
Flaky tests detected in 13fc935. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/5052140938
|
|
Perf test failure seems legit, looking into it |
|
So the failing perf test caught an actual bug: Post Content inner blocks show up as empty if the post is in draft mode. I just changed the logic a bit to detect the presence of content via the entity record instead of the block editor store, and it seems to be working correctly now. |
| [ postType, postId ] | ||
| ); | ||
|
|
||
| const hasInnerBlocks = !! entityRecord?.content?.raw; |
There was a problem hiding this comment.
Just looking above at the blocks value returned by useEntityBlockEditor — would it work to check if blocks.length is greater than 0?
Here's where blocks is determined:
There was a problem hiding this comment.
Aaaah yes that should work! Well spotted.
There was a problem hiding this comment.
Ok so it seems that blocks are also empty on first load, so for a draft post the content is getting replaced with the empty paragraph template we're passing useInnerBlocksProps 🤔 I think we'll have to stick with the entity record after all.
There was a problem hiding this comment.
Ah, gotcha! And it sounds like using .raw is probably safe because prior to accessing this, there's already a check to see whether or not the current user is allowed to edit the current entity (here) 👍
One last question, what would the difference here between using getEntityRecord and getEditedEntityRecord? Would the latter be preferable for any reason?
Something I'm finding odd in testing, is that if I access the empty page first, then the placeholder text is visible. However, if in Browse mode I first access a page with content, and then go to select the page without content, for some reason the placeholder text isn't visible until I click on the Post Content block. Here's a quick screengrab of going from a non-empty page to an empty page:
There was a problem hiding this comment.
what would the difference here between using
getEntityRecordandgetEditedEntityRecord? Would the latter be preferable for any reason?
I don't think so because here we're only interested in the initial state of the post as we open the editor, before any edits have happened. I guess getEditedEntityRecord would be relevant if we wanted to do something at some point after the post content had already been edited.
There was a problem hiding this comment.
Oh, I keep forgetting about my weird height value. I think that should be unrelated, but in my theme.json I have minimum height set to 70vh in my test environment:
"core/post-content": {
"dimensions": {
"minHeight": "70vh"
},
There was a problem hiding this comment.
I've re-tested with the minHeight styles removed, and it's still occurring for me. In the below screengrab I'm reloading on the Pages screen, then select a published page that contains a single Cover block. Then I click on the empty page, which is a published page:
2023-05-17.16.11.20.mp4
If I first go to the published empty page, then it displays the placeholder as expected 🤔
There was a problem hiding this comment.
Hmmm no, still can't reproduce it. Do you have any browser or CPU throttling going on or something? I can see the paragraph placeholder takes half a second to load, but it always appears regardless of where I'm navigating from.
There was a problem hiding this comment.
Hrm, no throttling that I'm aware of. It's very strange! It feels like for whatever reason the block isn't being triggered to re-render, or something in this context, I can't quite figure it out 🤔
When I log out the following values everything seems correct in the console:
console.log( 'entityRecord', entityRecord );
console.log( 'hasInnerBlocks', hasInnerBlocks );
And code-wise the change in this PR otherwise looks good to me.
There was a problem hiding this comment.
Oooooh got it: I can repro by setting the front page to blog. It doesn't happen if front page is set to a static page. Now to try and figure out why 😅
| const props = useInnerBlocksProps( | ||
| useBlockProps( { className: 'entry-content' } ), | ||
| { | ||
| value: blocks, | ||
| value: hasInnerBlocks ? blocks : undefined, |
There was a problem hiding this comment.
Ok for some reason, useEntityBlockEditor returns an empty array for blocks when the empty page is loaded after a non-empty page, but returns a single block if the empty page is loaded initially. I haven't dug much further - I'm guessing there's some bug in getEditedEntityRecord from which this originates.
The problem with blocks being an empty array is that innerBlocksProps.value evaluates to true here, which causes the whole thing to render as ControlledInnerBlocks, which returns empty because there are, in fact, no blocks 😅
So this change fixes that immediate issue, anyway. I think any further exploration is out of scope for this PR.
There was a problem hiding this comment.
(I guess an alternative/complementary fix would be to change the check in useInnerBlocksProps to innerBlocksProps.value?.length && innerBlocksProps.onChange but better not do that without more extensive testing 😅 )
There was a problem hiding this comment.
Good find digging into useInnerBlocksProps! I think there might still be an issue between the syncing state of passing in a value and the use of the template. When editing an empty page (using the post editor), if I go to the template view and start editing content directly in the post content block, with this PR applied, those changes don't appear to take effect. However in trunk they appear to 🤔
This PR:
2023-05-19.14.32.36.mp4
Trunk:
2023-05-19.14.37.52.mp4
I'm wondering if it's to do with the template approach attempting to use inner blocks directly instead of syncing to the entity record?
Just an idea, but for this block, I wonder if it'd be possible to use value instead of template — that is, if blocks is empty, could we pass the initialInnerBlocks to value instead of template? Not sure if that would create other issues, since it isn't really the synced content from the record 🤔
tellthemachines
force-pushed
the
add/paragraph-in-post-content
branch
from
9e2dc3c to
13fc935
Compare
Just for fun I tried this out, and it seems to address the issue @andrewserong reported: const initialInnerBlocks = [ createBlock( 'core/paragraph' ) ];
const props = useInnerBlocksProps(
useBlockProps( { className: 'entry-content' } ),
{
value: hasInnerBlocks ? blocks : initialInnerBlocks,
onInput,
onChange,
}
);I'm not clued into how omitting Sorry if it confuses the issue more! |
@ramonjd did you try adding inner blocks to Post Content in the site editor or the template editor after this change? When I tried this locally it didn't allow any edits. The problem seems to be that, while the block is being edited, I've reverted my previous change for now, as it seems messing with This brings us back to the bug Andy initially reported here, which is only reproducible if homepage is set to display latest posts. For the template paragraph to display inside Post Content, we need When Post Content has actual content, It would be great to get some eyes on this from someone who knows more about how |
Ah, good question 😄 I only tried adding text (no inner blocks) to the empty paragraph, so please ignore everything I said |
|
Since this PR has had several rounds of testing and the only remaining bug is the one mentioned above, which needs further investigation to determine both the cause and best approach to fix it, I'm going to go ahead and merge this PR, and open an issue detailing the bug in question so we can follow up separately. |
Sounds good, and this PR is an easy revert if it winds up causing any issues 👍 |
What?
Fixes #49086.
Adds an empty paragraph as template to inner blocks if the Post Content block doesn't already have content in it.
Testing Instructions
Testing Instructions for Keyboard
Screenshots or screencast