Add Terms Query block #70720
Add Terms Query block #70720
Conversation
mikachan
added
New Block
mikachan
requested review from
ajitbohra,
fabiankaegy,
juanmaguitar,
ndiego and
spacedmonkey
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. |
|
Size Change: +3.99 kB (+0.21%) Total Size: 1.94 MB
ℹ️ View Unchanged
|
|
@mikachan Are we sure that we want to utilize the I'm also not sure how the query arg |
Ah I see, in that case I think something like
Yes, these are essentially the same display option, and they can probably be consolidated. I was using |
Ah! I was convinced the block is behind this flag! So of course I agree @t-hamano, thanks for explaining. @mikachan, I think it would be good to do mark the block truly experimental to include it in the setting for the time being. I'd like to buy a bit more time to make sure we can extend the query from WooCommerce side so we can display terms contextually (e.g. subcategories of currently viewed category). |
|
This block, like the Query Loop block, is a bit more complex, so I'd like to ping the core team for visibility @WordPress/gutenberg-core. Any feedback is welcome. In summary, this PR is shipping two blocks: the Term Query and Term Template blocks. The design is similar to the Query Loop block, but a notable feature is that it uses the Block Bindings API to display term data. |
|
@mikachan, would you be open to merging "Order" and "Orderby" dropdowns? They occupy quite a lot of space while they can be easily combined into more compact single dropdown with options:
Also, I may be missing something but I don't see a use case for ordering by slug. How about removing it completely? I'm happy to provide a PR once someone confirms it's a good way to go. 🙌 |
@t-hamano I also didn't realise this, I thought that
@kmanijak Yes, this sounds like a good idea. Are there other components that use a similar pattern to this? Happy to remove ordering by slug too, sounds good! |
@t-hamano I've just been testing with the experimental block setting, and I'm seeing that any block that has |
|
I think I've figured out the experimental setting issue with this block 👀 Looks like the block was being included in the main array for all blocks rather than the experimental list. Should be fixed with 6e45569. |
|
@mikachan 
Edit: Nevermind, I've found the culprit in my own code! Note however that this does mean that setting both |
That's right. If we want to include the Term Query block in the block experiment setting, we need to add the block inside the By the way, I think that we also need to move the Term Template block. |
In my testing, the block is now only showing when experimental blocks are enabled. Are you still seeing the block when the experiment is turned off?
I've moved the Term Template block now as well: 47423d4. |
Co-authored-by: mikachan <mikachan@git.wordpress.org> Co-authored-by: cr0ybot <cr0ybot@git.wordpress.org> Co-authored-by: fabiankaegy <fabiankaegy@git.wordpress.org> Co-authored-by: scruffian <scruffian@git.wordpress.org> Co-authored-by: Bernie Reiter <96308+ockham@users.noreply.github.com> Co-authored-by: Ben Dwyer <ben@scruffian.com> Co-authored-by: ockham <bernhard-reiter@git.wordpress.org> Co-authored-by: Sarah Norris <sarah@sekai.co.uk> Co-authored-by: kmanijak <karolmanijak@git.wordpress.org> Co-authored-by: Aki Hamano <54422211+t-hamano@users.noreply.github.com> Co-authored-by: Karol Manijak <20098064+kmanijak@users.noreply.github.com> Co-authored-by: Cory Hughart <cory@alphaparticle.com>
|
Warning: Type of PR label mismatch To merge this PR, it requires exactly 1 label indicating the type of PR. Other labels are optional and not being checked here.
Read more about Type labels in Gutenberg. Don't worry if you don't have the required permissions to add labels; the PR reviewer should be able to help with the task. |
| } | ||
| }, | ||
| "style": "wp-block-term-template", | ||
| "variations": "file:./variations.js" |
There was a problem hiding this comment.
Is this needed for some reason, since we already pass variations in index.js?
There was a problem hiding this comment.
We were exploring what would allow the block to be easily extended, but we probably don't need this in the block.json file. We can remove it.
| isDefault: true, | ||
| icon: list, | ||
| scope: [ 'block', 'inserter' ], | ||
| innerBlocks: createInnerBlocks( __( 'Term Name with Count' ) ), |
There was a problem hiding this comment.
Why do we need different metadata in these variations? How is Term Name with Count different than Term Card?
There was a problem hiding this comment.
Term Card is being used in the grid view as a container for the term details, whereas Term Name with Count is more simple inline piece of metadata that includes only the term name and the count. The variations are being reworked in #71596, so this will probably change before being stabalised.
| */ | ||
| import TermsQueryContent from './terms-query-content'; | ||
|
|
||
| const TermsQueryEdit = ( props ) => { |
There was a problem hiding this comment.
Nit: this wrapper doesn't seem needed.
|
|
||
| export const settings = { | ||
| icon, | ||
| variations, |
There was a problem hiding this comment.
How come the variations are here and not in the parent block?
There was a problem hiding this comment.
Are they even needed to be in inserter scope too? If they really do, we should have different names and icons as it creates this:
| * @return {Object} The data fields object. | ||
| */ | ||
| function createDataFields( termDataValues, idValue ) { | ||
| return { |
There was a problem hiding this comment.
Sorry for asking all these post merge (that might have been already discussed), but why use block binding here?
What are the advantages over dedicated blocks like term name, term description or whatever and why do we need all these fields in the binding (ex link, parent etc..).
Now that we use paragraphs with bindings, these blocks are affected by global styles of paragraphs.
There was a problem hiding this comment.
We've had some discussions about this in the tracking issue and in PR #71596, I think we do likely need a dedicated term name block at least because of all the various ways it could be displayed (with link, with count, etc.) which is complicated to do with bindings.
There was a problem hiding this comment.
What are the advantages over dedicated blocks like term name, term description or whatever and why do we need all these fields in the binding (ex link, parent etc..).
We were mainly trying to avoid needing many new inner blocks (and it was good to see how far we could get with block bindings), but as @cr0ybot says, we'll likely end up creating a Term Name block to allow for more flexibility.
What?
Part of #49094.
This adds a new block called Terms Query, similar to the Query block but for terms rather than posts. It is designed to contain a new Terms Template block, which holds inner blocks with term data for displaying each term.
Why?
To allow for more options and flexibility when displaying terms.
How?
This block allows for inner blocks and has a wider potential for extensibility compared to the Categories/Terms List block.
I've tried to keep the implementation as simple as possible so we can build up the functionality as we add the other blocks. For the inspector controls, I've tried to stay as close as possible to the options provided by the Categories List block.
Testing Instructions
Screenshots or screencast