-
Notifications
You must be signed in to change notification settings - Fork 4.6k
Add Terms Query block patterns, nesting, and extensibility features #71596
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
Add Terms Query block patterns, nesting, and extensibility features #71596
Conversation
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
|
I've pushed a potential idea for addressing what we were discussing in this comment. I've added a new binding for the term name and link combined. Let me know what you think, feel free to revert/change! |
Hm, this seems dangerous. It works for a paragraph binding, but if this were to be mistakenly used in a button block with a link the HTML would be invalid. |
Oh yeah, true. I've tried adding a new boolean property to the block bindings, |
|
@mikachan I think we would need to check for the paragraph block as well to be safest, since other blocks may include a
Oh my god I just realized the |
|
A few things I'm noticing after some additional testing:
I'd like to take care of both in this PR but am open to being told that it's getting out of scope, but my primary mission contributing to this block is extensibility. |
Yeah, definitely. It got more complicated quickly (as most things do 😄). We can revert my attribute binding changes and move them to their own PR.
I think the extensibility of this block is very important, so definitely happy for it to be prioritised. I think it's also fine for them to be worked on in this PR. If it starts to feel like there are too many changes, we can always split the work up, but the first point you mention sounds like it should be included in this PR anyway. |
|
I removed the |
This is going to be more complicated than the query block. I've consolidated into a single
This is because Therefore, I admit that separate queries for child terms is actually preferable. However, I'd like to propose a different approach instead of the one currently in use: nested Terms Query blocks. We remove the This nested approach feels like it meshes well with the block editor ethos, allowing more control over the presentation of child terms from within the editor. It also follows more closely with the Query block, which does not have a I am going to proceed with this approach and update the patterns accordingly. |
This reverts commit a52096b.
|
@skorasaurus @mikachan I believe this is ready for review again. Inheritance should work for nested within other terms query blocks, nested in a query for a post's terms, and on a taxonomy template archive. One nested example is available as the "Category Children" pattern. The query is extensible via similar means to the query block with the I apologize that there are so many changes in this PR. I did refrain from implementing a few things like |
| id: { | ||
| label: __( 'Term ID' ), | ||
| value: idValue, | ||
| value: `${ idValue }`, |
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.
This fixed an error I was getting in the editor when using the term id binding for button text. Coercing to a string is probably not ideal in all cases though.
| $content = ( new WP_Block( $block->parsed_block ) )->render( array( 'dynamic' => false ) ); | ||
| remove_filter( 'render_block_context', $filter_block_context, 1 ); | ||
|
|
||
| return $content; |
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.
We don't need to render the container tag here since it's part of the static saved content in save.js.
| if ( $inherit ) { | ||
| if ( isset( $block->context['termQuery'] ) ) { | ||
| $query_context['taxonomy'] = $block->context['termQuery']['taxonomy'] ?? $query_context['taxonomy'] ?? 'category'; | ||
| } | ||
| if ( isset( $block->context['termId'] ) ) { | ||
| $query_context['parent'] = $block->context['termId'] ?? $query_context['parent'] ?? 0; | ||
| } | ||
| } | ||
|
|
||
| $filter_block_context = static function ( $context ) use ( $query_context ) { | ||
| $context['termQuery'] = $query_context; | ||
| return $context; | ||
| }; | ||
|
|
||
| // Use an early priority to so that other 'render_block_context' filters have access to the values. | ||
| add_filter( 'render_block_context', $filter_block_context, 1 ); | ||
| $content = ( new WP_Block( $block->parsed_block ) )->render( array( 'dynamic' => false ) ); | ||
| remove_filter( 'render_block_context', $filter_block_context, 1 ); |
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 introduced another layer of filtering block context so that the main block can handle termQuery inheritance. Otherwise, the template block only sees the termQuery context from it's container terms query block and not the parent terms query block in a nested scenario.
| <!-- wp:buttons --> | ||
| <div class="wp-block-buttons"> | ||
| <!-- wp:button {"metadata":{"bindings":{"url":{"source":"core/term-data","args":{"key":"link"}},"text":{"source":"core/term-data","args":{"key":"name"}}}}} --> | ||
| <div class="wp-block-button"><a class="wp-block-button__link wp-element-button">' . _x( 'Tag', 'Block pattern button text' ) . '</a></div> |
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.
This and other default button text were added because these buttons were not rendering without it. Not sure if this is an upstream issue with the button block or an issue with this PR. It does look like the button block is explicitly hidden if there is no set text content.
| "attributes": { | ||
| "namespace": { | ||
| "type": "string" | ||
| } | ||
| }, |
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.
The namespace attribute is only needed on the block that provides the query args for plugins to hook into.
|
@fabiankaegy could use your eyes on this one if you have any time to spare. |
|
|
||
| if ( empty( $query_block_context['termQuery'] ) ) { | ||
| return ''; | ||
| $query_args = gutenberg_build_query_vars_from_terms_query_block( $block ); |
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.
Any advice about how this is supposed to be handled? Getting a PHPCS error:
It's not allowed to call the "gutenberg_build_query_vars_from_terms_query_block()" function as its name matches the forbidden pattern: "/^[Gg]utenberg.*$/".
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 think this error means that we can't prefix the function name with gutenberg, so hopefully this error can be fixed by renaming the function to build_query_vars_from_terms_query_block.
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.
@mikachan I tried that and it failed, so not sure how to update the PR to fix this.
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.
Ah, so the check failed again even when you renamed the function? Do you know if you got the same error? I seem to be able to rename it locally on this branch OK, I can push that change and try it if you like, but I imagine it'll fail the check again if you already tried pushing this change.
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.
Sorry I misunderstood and renamed it where called instead of renaming the function. You can push the change if you'd like, I've just pushed it in my other PR #71916 so we'll see if that worked.
kmanijak
left a comment
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.
Hi @cr0ybot, thanks for this PR, especially about thinking of extensibility aspects that I really appreciate but also include some changes that I'm not a fan of.
Great things:
- make context/query filterable
- I love block bindings allowing to have a link to specific taxonomy!
- moving variation to Terms Query
Things to address:
- from my POV it has to be more flexible and work with custom taxonomies and custom templates. I'm speaking from WooCommerce perspective and will give WooCommerce examples:
- currently Terms Query breaks in editor with custom templates (e.g. in context of WooCommerce templates like Products by Categories/Tags but also Product Catalog or Single Product)
- "Default" Query Type doesn't work well with custom taxonomies (e.g Product Categories or Tags from WooCommerce)
- Terms Query works well with those use cases on
trunk
Things to discuss/consider:
- I was never a fan of "Query Type" (former "Inherit query from template") and I will strongly advocate for more explicit options addressing explicit use cases but that's a subject to discuss.
- I'm on the fence with Query Terms nesting... I must admit I initially didn't like it at all 😅 But as I played with child terms it makes more sense. I'm still thinking about it. But if we go this way, I'd hide most of the controls here (especially Query Type, Taxonomy) and only leave those that makes sense like "Show empty". They create confusion and I think block needs to make some decisions for users and hide complexity from them. What do you think? We can easily detect if block has Terms Query descendant.
- What happens to Term Count? Sorry if I missed that in the comments.
Happy to discuss this further! 🙌
| label: __( 'Most used' ), | ||
| value: 'count/desc', | ||
| }, | ||
| { | ||
| label: __( 'Least used' ), | ||
| value: 'count/asc', | ||
| }, |
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 not convinced to "Most/Least used" labels. They make me think of "popularity" and not necessarily count, especially that we don't know what taxonomy may represent. But I may be biased as I'm not a native English speaker.
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.
To be honest, former "Inherit Query from Template" and current "Query Type" controls are one of the most confusing ones I know.
It is confusing in Query Loop and I think it's even more confusing in Terms Query it's since:
- There's no global/default query to inherit from
- Inheritance order is strict here and I feel the "post content" can be error prone since
postIdis pretty commonly provided context that may easily lead to unexpected result.
// Inheritance order: block context, post context, taxonomy archive context.
I would honestly love more explicit options like a radio controls we had addressing specific use cases rather than Default vs Custom Query Type option.
In addition, "Default" option doesn't make much sense in many contexts in Editor, e.g in posts. I don't think we should put a responsibility of knowing how to set up a block to a user but rather do it for them. So at least the option should be hidden if not applicable but I'm personally against having an option in current shape.
| } | ||
| isSingular = singularTemplates.includes( templateType ); | ||
|
|
||
| return { isSingular, templateType, templateQuery }; |
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.
What do we need isSingular for? It's not used anywhere.
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 removed it.
| const templateTypeFromSlug = templateSlug.includes( '-' ) | ||
| ? templateSlug.split( '-', 1 )[ 0 ] | ||
| : templateSlug; | ||
| const queryFromTemplateSlug = templateSlug.includes( '-' ) | ||
| ? templateSlug.split( '-' ).slice( 1 ).join( '-' ) | ||
| : ''; | ||
| if ( queryFromTemplateSlug ) { | ||
| templateType = templateTypeFromSlug; | ||
| templateQuery = queryFromTemplateSlug; | ||
| } |
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 don't think this is generic enough as it assumes specific template slugs structure so it breaks on custom templates like this:
For example Woo has the following:
- products by category:
taxonomy-product_cat - for specific category:
taxonomy-product_cat-accessories - products by tag:
taxonomy-product_tag - for specific tag:
taxonomy-product_tag-red
I think we need to make it much more generic and maybe rely on templateSlug.includes( taxonomy ) where taxonomy is the one chosen in Terms Query block.
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.
@kmanijak Thanks for catching this. The function does work, I just used the wrong value for the taxonomy slug. PR has been updated.
| <BlockControls> | ||
| <TermsQueryToolbar | ||
| clientId={ clientId } | ||
| attributes={ attributes } | ||
| hasInnerBlocks={ false } | ||
| /> | ||
| </BlockControls> |
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.
What about NOT rendering this at all while in placeholder. User has already a choice so there's no need to display "Choose Pattern" button that does the same thing as "Choose" button.
| { !! hasPatterns && ! isSmallContainer && ( | ||
| <Button | ||
| __next40pxDefaultSize | ||
| variant="primary" | ||
| onClick={ openPatternSelectionModal } | ||
| > | ||
| { __( 'Choose' ) } | ||
| </Button> | ||
| ) } | ||
| { ! isSmallContainer && ( | ||
| <Button | ||
| __next40pxDefaultSize | ||
| variant="secondary" | ||
| onClick={ () => { | ||
| setIsStartingBlank( true ); | ||
| } } | ||
| > | ||
| { __( 'Start blank' ) } | ||
| </Button> |
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.
To be honest, I find this choice redundant in the Query Loop and here as well. "Choose" allow to choose pattern and more advanced use cases but "Start Blank" doesn't start blank but gives you another choice of "simple" patterns.
This is general comment that I'd rather put "Start Blank" options as first options in pattern selection modal since I don't see much value in giving users this choice.
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.
Yea, the wording "Start blank" feels like a vestige, not sure what would be better since I don't really think that actually starting blank is a good option. The difference is the choice between patterns (which can be supplemented by the pattern library) vs built-in simple variations. Maybe "Start simple"?
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.
Agreed! I like "Start simple" instead of "Start blank". We could open a follow-up to make the same change to the Query Loop block.
| // Maybe inherit taxonomy from parent query. | ||
| const taxonomyInherited = inherit && !! queryContext; | ||
| const taxonomy = taxonomyInherited | ||
| ? contextTaxonomy ?? queryContextTaxonomy ?? initialTaxonomy | ||
| : initialTaxonomy; | ||
|
|
||
| // Maybe inherit parent from parent query. | ||
| const parentInherited = inherit && !! termIdContext; | ||
| const parent = parentInherited ? termIdContext : initialParent || 0; |
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.
Could you explain that logic, what are the use cases here? 😅 Why isn't taxonomy deterministic?
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.
Mainly because taxonomy currently defaults to category. If it defaulted to an empty string maybe this could be simplified. This was done to attempt to automatically inherit the query if a terms query block is inserted within another.
Co-authored-by: Karol Manijak <[email protected]>
|
@kmanijak Thanks for taking a look!
Definitely think you've found some bugs if it's not working for custom taxonomies or templates. I will investigate.
I don't disagree with you, but I strongly believe that the two query blocks should work similarly, including controls. Since this PR doesn't aim to make changes to the Query Loop block, I've opted to use similar controls and wording where possible. I would be in favor of a separate PR that changes controls/wording in both blocks simultaneously.
I stopped short of implementing
I'm not sure I understand the question. The count can be applied via block bindings, but I'm also advocating for a new Term Title block that optionally includes the count. |
I see your point but I'm not convinced they necessarily have to have the same controls especially that "Query Type" is contextual and it makes more sense in Query Loop than in this block. While obviously they're very similar they also serve a different purpose and hence I think it's ok (or even expected) to have some differences that makes the use easier in this specific context. "Query Type" is already a result of attempt to improve it and... there was no better idea than this 😆 but control is partially enforced by technical aspect (there is global What about we display different option depending on the context. If we could determine the "inherit type" (from template, from post) in the Editor we could actually give more meaningful option to choose from, e.g. in taxonomy template: Query Type: From the template | Custom etc.
I get it. I know I'm late to the party but any chance this PR could be split into smaller chunks? There are some aspects that are probably good to go now while we could iterate on others like this one. I know we can always followup though.
Ah, my bad, I was asking about Term Count block not available but yeah, it was rather dummy question 😅 thanks for answering! And yeah, I saw discussion about dedicated block(s) instead of block bindings. 🙌 |
@kmanijak You're definitely right about this. Right now there is a static explanation for default: "Display a list of terms based on the current template or term.", which I did tailor a bit for this block. We might be able to make this more contextual.
😅 If that's what is desired here I can start over again... |
|
I'd really appreciate it if this PR could be broken down into smaller changes; it'd be easier to review and land 🙇 But I also appreciate being able to test all of these changes together. Is it possible to split some of the changes out on their own, without there being too much interdependency? |
|
@mikachan I think that the change to nesting & inheritance could be split out. I think this PR that adds patterns would have to be considered blocked because there was some organizational shuffling with the higher complexity of the edit component, plus some incompatibilities in the attributes and of course the nesting. I'll get started on that other PR hopefully tonight, and I'll probably open a separate issue for it as well. |
|
Closing in favor of smaller PRs, starting with #72197. |


This PR should be considered blocked by #71916, which splits out (and iterates on) the nesting & inheritance features from this PR. Once that is settled, this PR will be refocused on block patterns/variations.
What?
Addresses differences between the Query block's pattern and variation picker and the Terms Query block. This is a continuation of the effort from PR #71528 which was closed because the target branch was merged.
This PR removes variations from the term-template block, adds variations more similar to the query block to the terms-query block, and includes example patterns that can be chosen.
It also attempts to introduce extensibility mechanisms very similar to the core query block, which the pattern picker is a part of, but this has necessitated a refactoring of the query and hierarchical approach to allow only flat results with nesting accomplished via block nesting instead.
Why?
This was discussed in the main PR as one of the issues to address before releasing the terms-query block. Additionally, extensibility is critical for this block to be useful.
How?
hierarchicalquery argumentTesting Instructions
#3and confirm that choosing "start blank" presents the option to select from a few variationsTesting Instructions for Keyboard
All UI patterns are copied from the query block and should work the same
Screenshots or screencast
Additional Screenshots:
I am open to any feedback, and in particular am looking for thoughts about default patterns and variations.