Full Site Editing: Update the Content Slot block

[Copons]

Changes proposed in this Pull Request

• Rename the block from "Page Content" to Content Slot.
• Replace the temporary post selector approach (a simple select with the latest 10 posts) with an autosuggestion field roughly inspired by URLInput.

Note: the accessibility interactions between the input field and the popover should be improved.
Core handles keyboard events to move the cursor from one side to the other.
I have omitted it because it's a pretty good chunk of code, but we should take care of it in a follow up.
Note: the render callback for this block depends on the theme, and will be tackled in a follow-up.

Testing instructions

• Follow the README to build the plugin. (If the script fails, please run npm distclean and npm ci first (required because of a recent calypso-build change).
• In the block editor, try adding the Content Slot block.
• Type the title of any public entity (posts, pages, public CPTs), and make sure the results are as expected.
• Click on one of the suggestions: it should be displayed into the block.

Fixes #32492

[matticbot]

Test live: https://calypso.live/?branch=update/32492-fse-content-preview-block

[matticbot]

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.

[gwwar]

Am I missing something from readme steps? Trying to build the plugin I see:

Error: Cannot find module '@automattic/wordpress-external-dependencies-plugin'
    at Function.Module._resolveFilename (internal/modules/cjs/loader.js:581:15)
    at Function.Module._load (internal/modules/cjs/loader.js:507:25)
    at Module.require (internal/modules/cjs/loader.js:637:17)

wp-calypso/packages/calypso-build/package.json

Line 33 in 407b02e

"@automattic/wordpress-external-dependencies-plugin": "file:../wordpress-external-dependencies-plugin",

Edit: ah I think an extra npm install worked for me, but npm ci didn't. Do we need to regenerate the lock file or is this expected?

[gwwar]

Looks like a nice improvement @Copons As you noted there's a number of interactions we can tidy.

I'm not sure if my local WP environment is a bit messy, so if someone can confirm that this preview block loads after a publish/refresh and edit again without block invalidation I think this is good to 🚢

[dmsnell]

we probably want to make sure this throttle( apiFetch ) combination works the way we expect it to

the fetchAllMiddleware() might do some funky business here and we could potentially end up loading all the posts on the first few letters of autocomplete then proceed to reload a subset of those on and on until the end

we also probably want a debounce() instead of a throttle because we want the last-typed letters to trigger the last autocomplete search and throttle() might drop them if they were typed quickly after the previous query

[Copons]

You're correct, I've meant to use debounce and I've also forgot to add the wait time.
It looks to me it's working correctly, I can't find a case in which this would end up in an infinite loop of some sort. 🤔

(Also, the search is only triggered when the query is 2 or more characters long, and returns 20 results top)

[dmsnell]

returns 20 results top

this is strange to me. I can't figure out why it's the case but I can't get my search results to fetch all of the result pages. I see a proper Link header when not making calls to dotcom sites and the botched link header on dotcom sites. I thought that the way we mangle this header in our custom middleware replacements could be behind the behavior but I had trouble getting the auto-paging to work in a core-only test.

here's what I'm worried about: I think that it's a bug somewhere preventing the fetchAllMiddleware from continuing to pull in all successive search results, so even with per_page=20 it should fetch all the results automatically. That's my understanding, at least. I am not seeing the middleware do that but I'm at a loss for an explanation of why.

[Copons]

I'm not familiar with api-fetch, but I gave a look at fetchAllMiddleware and as it turns out it's only applied when the request explicitly asks for all results by "unbounding" the limit with per_page=-1:

const requestContainsUnboundedQuery = ( options ) => {
	const pathIsUnbounded = options.path && options.path.indexOf( 'per_page=-1' ) !== -1;
	const urlIsUnbounded = options.url && options.url.indexOf( 'per_page=-1' ) !== -1;
	return pathIsUnbounded || urlIsUnbounded;
};

// ...

if ( ! requestContainsUnboundedQuery( options ) ) {
	// If neither url nor path is requesting all items, do not apply middleware.
	return next( options );
}

https://github.com/WordPress/gutenberg/blob/774713c0467779a9651983ec96609a6637811132/packages/api-fetch/src/middlewares/fetch-all-middleware.js#L33-L37

https://github.com/WordPress/gutenberg/blob/774713c0467779a9651983ec96609a6637811132/packages/api-fetch/src/middlewares/fetch-all-middleware.js#L47-L50

[dmsnell]

what's the purpose of the await here?

updateSuggestions() will return a Promise under the hood but we're neither waiting on the resolution of that promise nor using its returned value, so I think we can disregard the await and the async in the function signature

[Copons]

Good catch.
It was a leftover from a temporary fix long gone. 🙂

[Copons]

I've rebased to the most recent master, which might fix @gwwar installation issue.

The new build system introduced in #32431 creates a new dist outside the plugin and theme folders.
Please delete the leftover dist folders inside the plugin and theme (it doesn't matter right now, nor it breaks anything, but eventually it would unnecessarily end up included in the build artifact), and update the symlink to the new locations (see: https://github.com/Automattic/wp-calypso/blob/master/apps/full-site-editing/README.md#local-development).

[Copons]

Working on the Content Renderer block I've realized that storing the entire post object in the block attributes means that:

1. The block is very long when viewed in code editor.
2. We are basically "locking" the block to the version of the post at the time of the selection.

In other words, if we select a post to preview, and then we edit that post, the preview would not be updated accordingly.
This would become a major inconvenience in the Content Renderer block, where is paramount to have to render the latest version of the selected post.

In f6d8ab7 I've slightly changed the selected post loading:

• Now the block only saves the post ID and type (required for the endpoint path).
• When we select a post, it gets saved in the block state, so that we keep viewing it during the current session without having to fetch it again.
• When we reload, if the state doesn't contain the selected post, we fetch it.
• Since now both the block and the post selector component are responsible to fetch the post, I've externalized the function into its own utility file.

Practically speaking, nothing visible changes.
The block footprint in code though, changes from something like
<!-- wp:a8c/content-preview {"selectedPost": { /* super long post object */ } } /-->
to
<!-- wp:a8c/content-preview {"selectedPostId":1234,"selectedPostType":"page"} /-->

[gwwar]

I may be misreading the code, but we'd probably want a dynamic block (have a render_callback to take over server rendering) here for this use case. Let me know otherwise if you had something else in mind.

To recap we

• don't want to embed a specific snapshot of post version content
• we want a minimal block representation of the post ( or post id/type), or the block will likely invalidate rather quickly
• if that preview post is updated, we'd expect our preview to change even if we didn't update the parent post.
• we can keep the API call for the editor, but wouldn't want this extra call on the published page

https://wordpress.org/gutenberg/handbook/designers-developers/developers/tutorials/block-tutorial/creating-dynamic-blocks/

[Copons]

@gwwar this block is supposed to be used in a template to mark where to render a post content.
In the editor we use it to preview how a post would look like. Eventually it will only be used in the template CPT, which is not merged yet (#32568).
In the front-end, it would be replaced by the post itself (how? one step at a time: this is twisted enough as it is! If needs arise, we can add a render_callback in a follow up).

We don't really need to store the preview post ID and type (actually, thinking about it, it probably makes things more confusing), but my tests felt smoother if the preview post persisted across reloads.
The fetch post on top of the edit function is to show the preview of the stored post, and it only runs once, when the editor loads, and only in the editor.
If the previewed post is changed when we are editing, the preview won't change unless we refetch it (e.g. by selecting it again from the post selector).

This is as opposed to the Content Renderer block (names could use some improvements 😄), introduced in #32581 (branched out from this PR).
The mechanics are 99% the same, with the main difference that Content Renderer actually renders something on the front-end (server-side via render_callback; no API calls).
If the rendered post changes, it will be automatically updated in the front-end as well at the next reload.

Content Renderer would be used to add layout elements (or anything really: why stop at header and footer when we might want to "embed" an entire post, say, in the sidebar?) to a template.

[vindl]

@gwwar this block is supposed to be used in a template to mark where to render a post content.
In the editor we use it to preview how a post would look like. Eventually it will only be used in the template CPT, which is not merged yet (#32568).
In the front-end, it would be replaced by the post itself (how? one step at a time: this is twisted enough as it is! If needs arise, we can add a render_callback in a follow up).

I agree with @gwwar regarding the dynamic block point, but if you'd prefer to do so, I think it's fine to handle that in a follow-up (we just need to make sure to write out the follow-up task for it). Regarding the "how" part, I think this was already handled in the early prototype here: https://github.com/dmsnell/stupid-code-do-not-use/blob/a8910a9720bb506841410c85b83429a1e932d5b2/fse/fse.php#L33.

We don't really need to store the preview post ID and type (actually, thinking about it, it probably makes things more confusing), but my tests felt smoother if the preview post persisted across reloads.

I think it makes things more confusing too - every template is supposed to be referenced by multiple posts down the road. Assigning a particular post_id to a generic placeholder can be unclear in this context.

This is as opposed to the Content Renderer block (names could use some improvements 😄), introduced in #32581 (branched out from this PR).
The mechanics are 99% the same, with the main difference that Content Renderer actually renders something on the front-end (server-side via render_callback; no API calls).
If the rendered post changes, it will be automatically updated in the front-end as well at the next reload.

Which makes me wonder whether we really need two separate block types for this? 😄They are confusing me now and I guess the difference won't be clear to users either. In light of my comments here #32581 (comment), we could consider merging this functionality into one block, that will behave differently depending on the context:

• editing context
  • it can only be added to wp_template CPT
  • if it references the wp_template (via suggestion dropdown) we pin it down to a particular id for it
  • if it references any other post type (scoped down to pages initially) it acts as a post_content placeholder
• rendering context
  • if we detect that it's referencing the wp_template post type, we render it's contents (like we do now in rendering callback for Content Renderer block)
  • if it's referencing some other post type (scoped down to pages initially), we defer rendering to parent post that referenced this template (#)

Having just one block might make things easier to grok conceptually (cc @dmsnell for thoughts). We could call it something like [Layout|Template] [part|component|element].

[Copons]

Ok, let's take a step back here. 🙂

I'm opposed to have 1 block for both functionalities.
It would be confusing for both the user (same block for slightly different outcomes? having to choose a template to do one thing, or a page to do another thing? seems complicated), and for us as well.
There are technical limitation (e.g. we want 1 single Content Preview, but allow multiple Content Renderer), and all in all I don't really see any downsides in having two slightly similar blocks, as long as we give them very distinct names.

So: I 100% agree that the current naming is totally messed up (tbh I wasn't really paying attention to it for now).
I'd say we could do this for now (numbered, but not in any particular order):

1. Keep both blocks on hold until the wp_template CPT ships.
2. Don't store anything in Content Preview: folks can preview how a post looks like in a template, but can't persist the preview across reloads.
3. Add a render callback to Content Preview. @dmsnell did this indeed (which is subtly different by what you linked @vindl 🙂 ), and I've misread the code, thinking it was handled by the theme.
4. Content Preview can't preview a Template (but any other public post type).
5. Content Renderer can only render wp_templates.
6. Limit both blocks to the wp_template CPT.
7. Rename the blocks into:
   • Content Preview -> Content Slot (or something better that I can't come up with)
   • Content Renderer -> Template Element (I'd avoid "Layout" because it's a core block category)

Am I missing anything?

[vindl]

Add a render callback to Content Preview. @dmsnell did this indeed (which is subtly different by what you linked @vindl 🙂 ), and I've misread the code, thinking it was handled by the theme.

Sorry, I now see that I grabbed a link to a wrong render callback. (got confused by those names once again! 😄)

[gwwar]

@Copons do you mind adding a readme file that states what this block is intended to be used for, why, and maybe add a quick example of serialized post_content.

[dmsnell]

at a later point in time or even now I think it's worth spitting some output here.
this is somewhat high-level and not a priority at this point, but one of our guiding principles is being part of the open web. I would love to see some fallback message here for the case where a post is viewed purely as HTML without WordPress the PHP engine rendering it.

could we think of a simple message to report that would convey "this is the spot where a post would normally show, but this page is being viewed without the necessary software so you get this instead 😄 "

[dmsnell]

oh. ha. see my above comment in the static render fallback. note that while we are free to render this here, we can also send that into the editor, and even have both. but if we have the static fallback render this isn't entirely necessary.

[dmsnell]

can we explain why this exists and why we aren't using getEntityRecord() to fetch the post?

[Copons]

I was under the impression that getEntityRecord was a simple selector, but in fact it has a resolver that apiFetches on its own.
I'll see to update this behaviour ASAP.