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

Block Library: Add Query block. #22199

Merged
merged 16 commits into from
May 13, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions lib/blocks.php
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,9 @@ function gutenberg_reregister_core_block_types() {
'post-tags.php' => 'core/post-tags',
'site-title.php' => 'core/site-title',
'template-part.php' => 'core/template-part',
'query.php' => 'core/query',
'query-loop.php' => 'core/query-loop',
Copy link
Member

Choose a reason for hiding this comment

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

Is there ever going to be a query without a loop? Could it be absorbed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What do you mean by absorbed? It needs to be movable against its pagination.

Copy link
Member

Choose a reason for hiding this comment

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

I thought pagination would sit outside in most cases

'query-pagination.php' => 'core/query-pagination',
Copy link
Contributor

Choose a reason for hiding this comment

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

In a single.html template i can see my self needing a similar pagination block (next/previous) but without having a wrapping query in this case since we're already on the "post" template page. Do you think there's a way to make that work?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, agreed — we discussed this briefly with examples of having pagination in the header, or a sidebar, etc.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, templates will have their query context provided from the root level so you could insert pagination under that.

)
);
}
Expand Down
30 changes: 22 additions & 8 deletions lib/class-wp-block.php
Original file line number Diff line number Diff line change
Expand Up @@ -169,19 +169,33 @@ public function __get( $name ) {
/**
* Generates the render output for the block.
*
* @param array $options {
* Optional options object.
*
* @type bool $dynamic Defaults to 'true'. Optionally set to false to avoid using the block's render_callback.
* }
*
* @return string Rendered block output.
*/
public function render() {
public function render( $options = array() ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this update needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

global $post;

$is_dynamic = $this->name && null !== $this->block_type && $this->block_type->is_dynamic();
$options = array_replace(
array(
'dynamic' => true,
),
$options
);

$is_dynamic = $options['dynamic'] && $this->name && null !== $this->block_type && $this->block_type->is_dynamic();
$block_content = '';

$index = 0;
foreach ( $this->inner_content as $chunk ) {
$block_content .= is_string( $chunk ) ?
$chunk :
$this->inner_blocks[ $index++ ]->render();
if ( ! $options['dynamic'] || empty( $this->block_type->skip_inner_blocks ) ) {
$index = 0;
foreach ( $this->inner_content as $chunk ) {
$block_content .= is_string( $chunk ) ?
$chunk :
$this->inner_blocks[ $index++ ]->render();
}
}

if ( $is_dynamic ) {
Expand Down
15 changes: 11 additions & 4 deletions packages/block-editor/src/components/block-preview/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -13,12 +13,15 @@ import { memo, useMemo } from '@wordpress/element';
* Internal dependencies
*/
import BlockEditorProvider from '../provider';
import LiveBlockPreview from './live';
import AutoHeightBlockPreview from './auto';

export function BlockPreview( {
blocks,
__experimentalPadding = 0,
viewportWidth = 700,
__experimentalLive = false,
__experimentalOnClick,
} ) {
const settings = useSelect( ( select ) =>
select( 'core/block-editor' ).getSettings()
Expand All @@ -29,10 +32,14 @@ export function BlockPreview( {
}
return (
<BlockEditorProvider value={ renderedBlocks } settings={ settings }>
<AutoHeightBlockPreview
viewportWidth={ viewportWidth }
__experimentalPadding={ __experimentalPadding }
/>
{ __experimentalLive ? (
Copy link
Contributor

@youknowriad youknowriad May 13, 2020

Choose a reason for hiding this comment

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

Is the difference here just to avoid the "scaling" ? Maybe we can have a "enableScaling" prop? "live" is a bit confusing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The scaling, block memoization, and other nuance.

<LiveBlockPreview onClick={ __experimentalOnClick } />
epiqueras marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if the onClick prop really needs to be supported in the preview component itself or more added to a wrapper "button" like we do for patterns.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is added to a wrapper. LiveBlockPreview is an internal component.

Copy link
Contributor

Choose a reason for hiding this comment

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

I was thinking more something like this:

<div role="button" onClick={doSomething}>
   <BlockPreview>
</div>

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That could work, but we lose the ability for BlockPreview to hook into it. I wonder if that would be useful. If by the time we have to remove the experimental prefix, a use-case hasn't arisen, then I guess we can move it outside.

) : (
<AutoHeightBlockPreview
viewportWidth={ viewportWidth }
__experimentalPadding={ __experimentalPadding }
/>
) }
</BlockEditorProvider>
);
}
Expand Down
24 changes: 24 additions & 0 deletions packages/block-editor/src/components/block-preview/live.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
/**
* WordPress dependencies
*/
import { Disabled } from '@wordpress/components';

/**
* Internal dependencies
*/
import BlockList from '../block-list';

export default function LiveBlockPreview( { onClick } ) {
return (
<div
tabIndex={ 0 }
role="button"
onClick={ onClick }
onKeyPress={ onClick }
>
<Disabled>
<BlockList />
</Disabled>
</div>
);
}
6 changes: 6 additions & 0 deletions packages/block-library/src/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,9 @@ import * as socialLink from './social-link';
// Full Site Editing Blocks
import * as siteTitle from './site-title';
import * as templatePart from './template-part';
import * as query from './query';
import * as queryLoop from './query-loop';
import * as queryPagination from './query-pagination';
import * as postTitle from './post-title';
import * as postContent from './post-content';
import * as postAuthor from './post-author';
Expand Down Expand Up @@ -197,6 +200,9 @@ export const __experimentalRegisterExperimentalCoreBlocks =
? [
siteTitle,
templatePart,
query,
queryLoop,
queryPagination,
postTitle,
postContent,
postAuthor,
Expand Down
5 changes: 5 additions & 0 deletions packages/block-library/src/query-loop/block.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
{
"name": "core/query-loop",
"category": "layout",
"context": [ "queryId", "query" ]
}
63 changes: 63 additions & 0 deletions packages/block-library/src/query-loop/edit.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,63 @@
/**
* WordPress dependencies
*/
import { useState, useMemo } from '@wordpress/element';
import { useSelect } from '@wordpress/data';
import {
BlockContextProvider,
InnerBlocks,
BlockPreview,
} from '@wordpress/block-editor';

/**
* Internal dependencies
*/
import { useQueryContext } from '../query';

const TEMPLATE = [ [ 'core/post-title' ], [ 'core/post-content' ] ];
export default function QueryLoopEdit( { clientId, context: { query } } ) {
const [ { page } ] = useQueryContext();
const [ activeBlockContext, setActiveBlockContext ] = useState();

const { posts, blocks } = useSelect(
( select ) => ( {
posts: select( 'core' ).getEntityRecords( 'postType', 'post', {
...query,
offset: query.per_page * ( page - 1 ) + query.offset,
page,
} ),
blocks: select( 'core/block-editor' ).getBlocks( clientId ),
} ),
[ query, page, clientId ]
);

const blockContexts = useMemo(
() =>
posts?.map( ( post ) => ( {
postType: post.type,
postId: post.id,
} ) ),
[ posts ]
);
return blockContexts
? blockContexts.map( ( blockContext ) => (
epiqueras marked this conversation as resolved.
Show resolved Hide resolved
<BlockContextProvider
key={ blockContext.postId }
value={ blockContext }
>
{ blockContext ===
( activeBlockContext || blockContexts[ 0 ] ) ? (
epiqueras marked this conversation as resolved.
Show resolved Hide resolved
<InnerBlocks template={ TEMPLATE } />
Copy link
Contributor

@youknowriad youknowriad May 13, 2020

Choose a reason for hiding this comment

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

I'd have expected this to be a "controlled" InnerBlocks and the template being saved to an attribute.
The reason being that if we save the template as InnerBlocks, the markup saved (fallback) is not a valid representation of what we want to show, it's just one post.

Copy link
Member

Choose a reason for hiding this comment

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

Wouldn't it save the actual template representation (post-title, post-content, etc), not a specific post?

Copy link
Contributor

Choose a reason for hiding this comment

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

yes, it does. I guess maybe it's fine since most of these are dynamic blocks. But you can end up with groups...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, it just saves the actual template representation.

) : (
<BlockPreview
blocks={ blocks }
__experimentalLive
__experimentalOnClick={ () =>
setActiveBlockContext( blockContext )
}
/>
) }
</BlockContextProvider>
) )
: null;
}
28 changes: 28 additions & 0 deletions packages/block-library/src/query-loop/index.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
/**
* WordPress dependencies
*/
import { __ } from '@wordpress/i18n';
import { loop } from '@wordpress/icons';

/**
* Internal dependencies
*/
import metadata from './block.json';
import edit from './edit';
import save from './save';

const { name } = metadata;
export { metadata, name };

export const settings = {
title: __( 'Query Loop' ),
icon: loop,
parent: [ 'core/query' ],
Copy link
Contributor

Choose a reason for hiding this comment

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

What If I want to add a "group" between the query and the loop, it seems like a valid use-case for styling/layout.
It seems like there might be a need for something like "parent" but works across levels.

Copy link
Member

Choose a reason for hiding this comment

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

Wouldn't the group be added outside the "query" in that case?

Copy link
Contributor

Choose a reason for hiding this comment

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

Sometimes you need it to be inside depending on the layout and how you want to place the other "query blocks" (pagination).

Say you want a background around loop, and a different background around pagination and potentially add some text (paragraph block) between them as a title for "pagination"

I don't think it's a blocker for this PR but I do think we'd need advance use-cases like that.

Copy link
Member

Choose a reason for hiding this comment

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

I guess I've been thinking of query and loop as a single unit, with pagination sitting outside and inheriting context globally (since it needs to be reflected in the url structure). Related: #22199 (comment)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I plan on removing parent once we implement a root query context derived from the URL and template.

At that point, you can have the loop and pagination blocks at any level of the document. The Query block is just for setting your own custom queries.

supports: {
inserter: false,
reusable: false,
html: false,
},
edit,
save,
};
58 changes: 58 additions & 0 deletions packages/block-library/src/query-loop/index.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,58 @@
<?php
/**
* Server-side rendering of the `core/query-loop` block.
*
* @package WordPress
*/

/**
* Renders the `core/query-loop` block on the server.
*
* @param array $attributes Block attributes.
* @param string $content Block default content.
* @param WP_Block $block Block instance.
*
* @return string Returns the output of the query, structured using the layout defined by the block's inner blocks.
*/
function render_block_core_query_loop( $attributes, $content, $block ) {
$page_key = 'query-' . $block->context['queryId'] . '-page';
$page = empty( $_GET[ $page_key ] ) ? 1 : filter_var( $_GET[ $page_key ], FILTER_VALIDATE_INT );
$posts = get_posts(
array(
'post_type' => 'post',
'posts_per_page' => $block->context['query']['per_page'],
'offset' => $block->context['query']['per_page'] * ( $page - 1 ) + $block->context['query']['offset'],
)
);

$content = '';
foreach ( $posts as $post ) {
$content .= (
new WP_Block(
$block->parsed_block,
array_merge(
$block->available_context ? $block->available_context : array(),
Copy link
Member

Choose a reason for hiding this comment

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

available_context is a protected property, meaning that it can't be accessed externally like this. Which is intentional, as otherwise the context property of a block registration is meaningless if all context can be accessed directly anyways. Which isn't to say we couldn't just decide that to be the case, but as things stand, you won't be able to do this sort of "passing along".

I might wonder instead some option of:

  • Avoiding to be recreating the rendering, somehow allowing for a continuation of the normal render flow of inner blocks, simulated as if (or in reality of) these being inner blocks of the block
  • Some expression of a block like query loop to be able to receive all context (e.g. "context": "*", "context": true)
  • Not passing along the ancestry context
  • Make available_context as visibility public (with all caveats noted above and likely resulting refactoring)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why can't available_context be public?

The context registration doesn't become meaningless; they are just used for different things.

Avoiding to be recreating the rendering, somehow allowing for a continuation of the normal render flow of inner blocks, simulated as if (or in reality of) these being inner blocks of the block

I don't see how we would clone the blocks with different contexts for every post.

Some expression of a block like query loop to be able to receive all context (e.g. "context": "*", "context": true)

That would make the block rerender whenever unrelated context changes just to hack around making available_context public.

Not passing along the ancestry context

We need it.

Copy link
Member

Choose a reason for hiding this comment

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

The context registration doesn't become meaningless; they are just used for different things.

Why should we expect a developer to implement context, if they could save themselves the effort by opting not to, and still have access to the same values?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because context subscribes the block to rerender when one of those values change.

In the future, it can also be used to scope blocks so that they can only be inserted inside blocks where the required context is present.

Copy link
Member

Choose a reason for hiding this comment

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

Because context subscribes 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.

Copy link
Member

Choose a reason for hiding this comment

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

In the future, it can also be used to scope blocks so that they can only be inserted inside blocks where the required context is present.

Yeah, static analysis is useful for things like that. I don't see how exposing available_context for advanced use cases detracts from that.

Because we have no guarantees they won't just use available_context for all context values. It's one less step they can save. There's an incentive to avoid effort.

#22199 (comment)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That would be going out of their way to do things the wrong and undocumented way, and in that case, they would lose the benefits of static analysis. I don't think it's a problem.

Copy link
Member

Choose a reason for hiding this comment

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

It's not going out of their way if it's the default state of inaction. I think it would be quite probable that one could stumble their way to $block->available_context, see some values they want to use, and never even realize that a "context" block type property even exists. Especially for things like postId and postType.

they would lose the benefits of static analysis

What are these benefits?

Related to this, part of the question to me as well is: Should we really care about context?

I'm not comfortable to just push forward with something like #22334, since I think it defers answering fundamental questions about what how we want context to work, in a way which doesn't oblige us to ever revisit it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Interesting challenge here. It's also related somehow to the fact that we store the template as "inner blocks" and not an attribute which for me is not ideal conceptually since these are not real inner blocks. It looks like we're doing a lot of magic here where IMO the solution should be something like:

do_blocks( template, context )

Where we set the post context as aa second argument. I'd even prefer if we avoid passing the parent context entirely, it's not clear at this point in which use-cases it's needed but if we ever have to support it context = * seems like a good approach to me.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Interesting challenge here. It's also related somehow to the fact that we store the template as "inner blocks" and not an attribute which for me is not ideal conceptually since these are not real inner blocks. It looks like we're doing a lot of magic here where IMO the solution should be something like:

That's not related to available_context. I tried storing the template in an attribute, and it was a lot more complicated and less performant.

Where we set the post context as aa second argument. I'd even prefer if we avoid passing the parent context entirely, it's not clear at this point in which use-cases it's needed but if we ever have to support it context = * seems like a good approach to me.

You're right in that we might not even need to pass down available_context context. @aduth I'll update it so that we don't use available_context at all for now.

Copy link
Member

Choose a reason for hiding this comment

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

Noting that, as merged, this code won't work as intended.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can we make it public for now without committing to it in terms of backward compatibility?

array(
'postType' => $post->post_type,
'postId' => $post->ID,
)
)
)
)->render( array( 'dynamic' => false ) );
}
return $content;
}

/**
* Registers the `core/query-loop` block on the server.
*/
function register_block_core_query_loop() {
register_block_type_from_metadata(
__DIR__ . '/query-loop',
array(
'render_callback' => 'render_block_core_query_loop',
'skip_inner_blocks' => true,
)
);
}
add_action( 'init', 'register_block_core_query_loop' );
8 changes: 8 additions & 0 deletions packages/block-library/src/query-loop/save.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
/**
* WordPress dependencies
*/
import { InnerBlocks } from '@wordpress/block-editor';

export default function QueryLoopSave() {
return <InnerBlocks.Content />;
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be "null"? Isn't this a dynamic block?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, but its inner blocks are the template for the loop.

}
5 changes: 5 additions & 0 deletions packages/block-library/src/query-pagination/block.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
{
"name": "core/query-pagination",
"category": "layout",
"context": [ "queryId", "query" ]
}
52 changes: 52 additions & 0 deletions packages/block-library/src/query-pagination/edit.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,52 @@
/**
* WordPress dependencies
*/
import { Button, ButtonGroup } from '@wordpress/components';
import { chevronLeft, chevronRight } from '@wordpress/icons';
import { __ } from '@wordpress/i18n';

/**
* Internal dependencies
*/
import { useQueryContext } from '../query';

export default function QueryPaginationEdit( {
context: {
query: { pages },
},
} ) {
const [ { page }, setQueryContext ] = useQueryContext();

let previous;
if ( page > 1 ) {
previous = (
<Button
isPrimary
icon={ chevronLeft }
onClick={ () => setQueryContext( { page: page - 1 } ) }
>
{ __( 'Previous' ) }
</Button>
);
}
let next;
if ( page < pages ) {
next = (
<Button
isPrimary
icon={ chevronRight }
onClick={ () => setQueryContext( { page: page + 1 } ) }
>
{ __( 'Next' ) }
</Button>
);
}
return previous || next ? (
<ButtonGroup>
{ previous }
{ next }
</ButtonGroup>
) : (
__( 'No pages to paginate.' )
);
}
24 changes: 24 additions & 0 deletions packages/block-library/src/query-pagination/index.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
/**
* WordPress dependencies
*/
import { __ } from '@wordpress/i18n';

/**
* Internal dependencies
*/
import metadata from './block.json';
import edit from './edit';

const { name } = metadata;
export { metadata, name };

export const settings = {
title: __( 'Query Pagination' ),
parent: [ 'core/query' ],
youknowriad marked this conversation as resolved.
Show resolved Hide resolved
supports: {
inserter: false,
reusable: false,
html: false,
},
edit,
};
Loading