Conversation
epiqueras
requested review from
Soean,
TimothyBJacobs,
ajitbohra,
ellatrix,
jorgefilipecosta,
talldan and
youknowriad
as code owners
|
Size Change: +1.76 kB (0%) Total Size: 833 kB
ℹ️ View Unchanged
|
| 'site-title.php' => 'core/site-title', | ||
| 'template-part.php' => 'core/template-part', | ||
| 'query.php' => 'core/query', | ||
| 'query-loop.php' => 'core/query-loop', |
There was a problem hiding this comment.
Is there ever going to be a query without a loop? Could it be absorbed?
There was a problem hiding this comment.
What do you mean by absorbed? It needs to be movable against its pagination.
There was a problem hiding this comment.
I thought pagination would sit outside in most cases
| add_query_arg( $page_key, $page + 1 ) | ||
| ); | ||
| } | ||
| return $content; |
There was a problem hiding this comment.
Is this block meant to handle post navigation when on single contexts as well?
If so, it'd make sense to replicate or borrow some of https://developer.wordpress.org/reference/functions/get_the_post_navigation/
There was a problem hiding this comment.
I think these queries/paginations need to be independent of their surrounding template's 'default' context, since they could otherwise 'break out' of the template -- e.g. we could have a query in a post that shows a category archive, in which case the prev/next navigation needs to go through that category's pages, rather than the adjacent posts.
(I hope I understood your question correctly 😅 )
We should however probably make sure to use the 'proper' navigation for the template's default context. So if we're editing a category archive template, and we use an (implied or explicit) query at the top level, we should make sure that the next/prev buttons use the 'correct' links to the next/prev page in the archive. (Edit: See e.g. #19257 (comment) for context)
There was a problem hiding this comment.
If so, it'd make sense to replicate or borrow some of
Yeah, we can't use the functions directly because we can't easily manipulate the generated links, but we can borrow.
I think these queries/paginations need to be independent of their surrounding template's 'default' context
The Query block can set its own query or inherit the global one.
| $content = ''; | ||
| if ( 1 !== $page ) { | ||
| $content .= sprintf( | ||
| '<div><a href="%s">Previous</a></div>', |
There was a problem hiding this comment.
"Next" and "Previous" strings should be translatable and customizable in the editor.
There was a problem hiding this comment.
I made it translatable, but customization should be done in follow-ups.
| } | ||
| if ( $page < $block->context['query']['pages'] ) { | ||
| $content .= sprintf( | ||
| '<div><a href="%s">Next</a></div>', |
There was a problem hiding this comment.
The div seems superfluos wrapping the link?
There was a problem hiding this comment.
The styles that align the content in the front end need it.
|
@epiqueras here is a first pass at the loop icon, we need to polish it but it's a good start. File in Figma
|
|
Love the icon. It's a nice homage to "The Loop" :) |
I think my gut reaction to seeing this was that editing each post's layout inside of the Query Loop block kind of calls for using a template part. Should we maybe consider equipping the Query Loop block with a UI to load a template part (in a follow-up PR)? (And define entity specific defaults? E.g. a post template part. This would need to happen on a per-theme basis I guess, and we'd need to provide fallbacks.) |
I'm not totally sure I understand the problem(s) -- is this about that child Post Content block potentially including itself? Is that something that can happen when the query part is used inside of a template, or only when it's instead used in the post editor (in a post/page/content-type CPT)? If the latter, would an alternative solution be to remove certain blocks (such as Query) from the post editor's block picker? (Just trying to get a grasp of the problem, and maybe avoid the |
The only reason you would use a template part here is if you want to reuse the same query layout across different query blocks with different queries. An advanced use case that can be achieved manually and should not be a default.
Inner blocks are mapped before the block's callback is called, so we end up rendering inner blocks twice, once with the parent context, and once with the overridden one. If the parent context is a template part, reusable block, or content CPT, then it would recurse forever as the post content blocks would start rendering from the top again. |
|
^^That's actually better for performance as well. |
|
@pablohoneyhoney The icon looks great. Added! |
| new WP_Block( | ||
| $block->parsed_block, | ||
| array_merge( | ||
| $block->available_context ? $block->available_context : array(), |
There was a problem hiding this comment.
Because
contextsubscribes the block to rerender when one of those values change.
But that's only relevant for the client-side, and still doesn't give us any guarantees that context will be provided as an accurate set of properties, especially if they're not using the context values in the editor and have no need for it. It also introduces an inconsistency where a server-side block instance has an additional property (available_context) which has no equivalent in the browser.
| new WP_Block( | ||
| $block->parsed_block, | ||
| array_merge( | ||
| $block->available_context ? $block->available_context : array(), |
There was a problem hiding this comment.
Maybe there's something to explore here as an equivalent to React's Portal, in how it allows for the creation of a new tree of elements while retaining all of the existing context.
| */ | ||
| function render_block_core_query_pagination( $attributes, $content, $block ) { | ||
| $page_key = 'query-' . $block->context['id'] . '-page'; | ||
| $page = empty( $_GET[ $page_key ] ) ? 1 : $_GET[ $page_key ]; |
There was a problem hiding this comment.
We should probably be sanitizing these inputs, as they would be very easy to manipulate in ways we might forget to anticipate in the future (i.e. injection).
e.g.
| $page = empty( $_GET[ $page_key ] ) ? 1 : $_GET[ $page_key ]; | |
| $page = empty( $_GET[ $page_key ] ) ? 1 : filter_var( $_GET[ $page_key ], FILTER_VALIDATE_INTEGER ); |
To a more general point, typically it should be a red flag to need to be manually validating values like this. Which could be a sign of how we're reaching beyond the block's own attributes and context to query parameters of the URL. I don't know that there's much of an alternative, but it could give us some headaches in the future, or at least need to be especially cautious about. It's partly why functions like get_query_var exist, even if something akin to a query argument getter, providing value by preventing people from shooting themselves in the foot.
There was a problem hiding this comment.
I updated it.
Yeah, we should explore having our own safe utilities for this.
Co-authored-by: Andrew Duthie <[email protected]>
Co-authored-by: Andrew Duthie <[email protected]>
epiqueras
force-pushed
the
add/query-block-2
branch
from
0dd6696 to
30e0684
Compare
| new WP_Block( | ||
| $block->parsed_block, | ||
| array_merge( | ||
| $block->available_context ? $block->available_context : array(), |
There was a problem hiding this comment.
Noting that, as merged, this code won't work as intended.
There was a problem hiding this comment.
Can we make it public for now without committing to it in terms of backward compatibility?
7 participants
Description
This PR introduces a new Query block, which can be described as the block version of the WordPress post loop. Right now, it only supports basic query parameters, but the goal is for it to expose all the relevant parameters of a WP query so that it can be used in any template that needs to render lists of posts.
The implementation has a parent Query block that manages the query and can contain any combination of inner blocks.
Inside the Query block, the Query Loop block reads the query from context and uses it to render the resulting posts using its inner blocks as layout. In the editor, this is achieved by rendering the selected post using
InnerBlocksand the others using a new liveBlockPreview. This lets us switch between editing the layout as used by each different post and have it sync across all of the posts.The Query Pagination block similarly reads the query from context but just uses it to render pagination buttons. In the future, it could be expanded to have its own inner blocks so that you can more freely position the different components of a pagination UI.
All of the new block APIs enabled the implementation to go smoothly. Still, I did have to add a couple of things to the server rendering process: A way to skip inner blocks when rendering a block so that the Query Loop block doesn't recurse infinitely when it has a child Post Content block, and a way to skip dynamic rendering so that the Query Loop block can call its render function without recursing.
This PR is merely the big chunk infrastructure needed for the block, and there is still a lot of UI work to be done in follow-ups. Namely, the design and development of the query toolbar and sidebar controls.
How to test this?
With the full site editing experiment enabled and some published posts, insert a query block into a new post and play around with the pagination settings while making sure the editor and front end outputs match each other.
Try adding content to the templates and make sure they sync. Also, try moving around the Query Loop and Query Pagination blocks.
Screenshots
Types of Changes
New Feature: There is a new set of query (post loop) blocks for full site editing.
Checklist: