Terms Query: simplify controls and layout #72121
Conversation
This reverts commit e3c03e7.
cr0ybot
requested review from
ajitbohra,
fabiankaegy and
juanmaguitar
as code owners
|
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. |
# Conflicts: # docs/reference-guides/core-blocks.md # packages/block-library/src/term-template/block.json # packages/block-library/src/term-template/index.php # packages/block-library/src/terms-query/block.json # packages/block-library/src/terms-query/edit.js # packages/block-library/src/terms-query/edit/index.js # packages/block-library/src/terms-query/inspector-controls/display-options.js
|
Tagging @mikachan, @t-hamano, and @ntsekouras for review since I can't request reviewers. |
mikachan
added
[Type] Enhancement
How it's handled in trunk right now? Do we have the same issue or it's visible here with your changes? I'm trying to grasp the problem and the naming of REST params and Query params make it a little hard..
What changes do you think we should make in the REST API? Definitely out of scope here. |
packages/block-library/src/terms-query/edit/inspector-controls/empty-terms-control.js
Outdated
Show resolved
Hide resolved
Can you explain this a bit more or share a small recording? Was it there on trunk as well? What I tried was have the blocks with |
| checked={ value } | ||
| onChange={ ( inherit ) => | ||
| onChange( { | ||
| inherit, |
There was a problem hiding this comment.
I'm a little confused by this. Was inherit in trunk or added here? I cannot see it block.json either.
There was a problem hiding this comment.
This is part of making the extra termsToShow attribute redundant because it can all be handled by termQuery properties, and is an inbetween to supporting inheritance fully. This preserves the subterms functionality of termsToShow.
There was a problem hiding this comment.
Added inherit=false to the default termQuery object in block.json.
| value={ orderBy + '/' + order } | ||
| onChange={ ( value ) => { | ||
| const [ newOrderBy, newOrder ] = value.split( '/' ); | ||
| onChange( newOrderBy, newOrder ); |
There was a problem hiding this comment.
Nit: we could pass the object ready for update.
| // We set parent only when inheriting from the taxonomy archive context or not | ||
| // showing nested terms, otherwise nested terms are not displayed. | ||
| if ( | ||
| isset( $query['inherit'] ) |
There was a problem hiding this comment.
This might be related to my inherit comments. There shouldn't be a value here right now..
ntsekouras
left a comment
ntsekouras
left a comment
There was a problem hiding this comment.
Thanks for the PR! I've left some comments but overall this looks good!
I haven't checked in depth the php changes in term-template, but with all the manual testing I did, it seemed to work well.
Yes, the output is identical when these trunk settings are used: 
And as a side note, setting max terms to "0" in trunk with the rest of the settings the same results in this discrepancy between the editor and front end:
Definitely feeling that too 😅. I have been working under the wrong assumption about the
We could add a |
Yes, this is the functionality that
Do those categories have associated posts or did you set "Show empty terms" to true? My test of this functionality works as expected:
This is one area where a discrepancy between editor and front end is expected unless we do some additional work to query for a term to set as parent in the editor, and that could have its own flaws if the term we get has no children (making editing the term template difficult). In my other PR I am detecting the current term if the template is for a specific term archive and fetching it as parent so that the terms are correct for both editor and front end. |
ntsekouras
left a comment
ntsekouras
left a comment
There was a problem hiding this comment.
Let's get this in and iterate. Thanks!
Co-authored-by: cr0ybot <[email protected]> Co-authored-by: ntsekouras <[email protected]>
3 participants
What?
Addresses a request to split #71916 further as a smaller PR.
This PR greatly simplifies the query and display of terms by removing hierarchical layouts in favor of a flat list in preparation for further block nesting and inheritance. It also removes a redundant attribute and reorganizes the edit component into its own folder, and controls have been refactored to have a single responsibility.
Why?
I've opened a string of PRs that have been too large and complex, starting with an attempt at adding a pattern/variation picker in which I suggested that the hierarchical rendering was causing more problems than it solved, and we already have a Term List block. In the PR attempting to address nesting and inheritance, I was asked to split it further.
How?
termsToShowattribute in favor of handling the same functionality with thetermQueryattribute/context.Testing Instructions
The block(s) should function the same as before, however the "Show subterms only" setting has been relocated to "Inherit parent term from taxonomy archive" without backwards-compatibility with the removed
termsToShowattribute.Some notable test cases:
parentis set (when inheriting).Screenshots or screencast