[Block Library - Query Loop]: Add allowedControls in block variations for better extensibility
#43632
Conversation
ntsekouras
added
[Type] Enhancement
|
Size Change: +796 B (0%) Total Size: 1.25 MB
ℹ️ View Unchanged
|
ntsekouras
force-pushed
the
try/query-loop-variations-meta
branch
from
9783230 to
7f75edc
Compare
ntsekouras
changed the title
meta attribute for better extensibility
ntsekouras
force-pushed
the
try/query-loop-variations-meta
branch
from
7f75edc to
77dd0f0
Compare
ntsekouras
changed the title
allowedControls in block variations for better extensibility
ntsekouras
requested review from
ajitbohra,
ryanwelcher,
juanmaguitar and
fabiankaegy
as code owners
|
Nice exploration @ntsekouras. I think it's pretty close to how we could offer the control over which controls related to the query should be used by a given block variation. The bonus would be that every site owner could tweak which controls can be used by users. I like how this PR shows how every Query block variation can go about ensuring it's the one currently applied. You presented how including an extra attribute As for using {
"supports": {
"typography": {
"fontSize": true,
"lineHeight": true,
"__experimentalFontFamily": true,
"__experimentalFontStyle": true,
"__experimentalFontWeight": true,
"__experimentalLetterSpacing": true,
"__experimentalTextTransform": true,
"__experimentalDefaultControls": {
"fontSize": true,
"fontAppearance": true,
"textTransform": true
}
}
}
}It would be nice to use a similar shape to enable controls and pick which controls are available and which are shown in the default view in the sidebar: {
"supports": {
"query": {
"order": true,
"taxQuery": true,
"search": true,
"__experimentalDefaultControls": {
"order": true
}
}
}
}It's very likely that to get use there we would have to figure out how to merge |
|
Really liking the flexibility this PR opens up @ntsekouras 👏🏻
Another (potential?) advantage of the namespace is it allows for alternative variations for the same postType.
I'm not sure about this. supports infers not just enabling controls but that the block also supports them. In the case of query block variation, I'm assuming any variations would technically still support the control but just want to limit what the user is able to modify via UI? In that case, would supports be semantically correct? |
Another thought here is the potential for more granular permission based availability. |
|
Great work on this PR as well! Should I close my PR in favor of this one? I mean, I'm still a bit partial to some of my implementation details (also because I spent a lot of time on that 😬), of course, but nothing I hold too strongly and couldn't throw away in favor of something y'all feel it would be a better overall direction for the project. I like the idea of using the Overall, I think the semantic ambiguity is a price we can pay for the advantages we gain. So, let me know if there is something we should salvage from my PR, or we should outright close it, or how I can help with this implementation. |
@nerrad, it's a gray area with block variations. If you look at that historically, we had individual blocks that shared most of the characteristics, e.g. Embeds, Social Links. We went and combined them under one block type and introduced variation so we could still offer the same customizations. I would say that conceptually those are different blocks, but grouped together to optimize for code reuse and to offer additional UI behaviors that for some blocks offer easier switching between different variations. I agree that The one more thing to think about is that names used here like
No need to close your PR for now. It's great to have a few ideas floating around so we pick the best approach later in the process. The work you started is a great way to verify also the ideas in practice. |
|
Thanks for the feedback all!
In this PR the
I agree with @nerrad and I wouldn't prefer them to be in It's not only about semantics though for me. I believe that we can constraint this really specific functionality for this complex block without adding more complexity with props for each control etc... Also even if we did it in
This is the drawback from this approach, but I think we don't really need it(at least for now). Query Loop exists for a while now and most issues are about adding more filters/options to it and refinements to 'lighten' the UI are happening(ToolsPanel) and are going to happen. |
Just a little note on this. I used the negative approach in my PR (in the form of |
This is handled here. |
packages/block-library/src/query/edit/inspector-controls/index.js
Outdated
Show resolved
Hide resolved
I'm not sure we'll need one, but 🤷 . The other cases usually mentioned(like the
@gziolo what did you have in mind with the above? Something in GB? |
This is one of the most requested features that is still missing in the block editor to be able to disable all or individual UI controls in the block toolbar and inspector controls. It's now possible to UI controls related to Styles, but it's missing to block-specific controls as it's far more difficult to integrate.
I meant that you could in theory extend |
Wow, I wasn't aware of that.. |
Here is the most recent lightning talk "Customizing Core Blocks" by Alex Ball from WordCamp US that should give you an idea about people's use cases: https://youtu.be/-hWtGDLc_Nk?t=15539. |
@ntsekouras If you check out the conversation in #42079, opinions surrounding using new For some folks, the ability to customize these controls is the difference between using a core block and making a custom one; I'd love to see more tooling become available to reduce the need for custom blocks that closely resemble core ones. The work you're doing here looks like it could really fill this need were the scope expanded a bit. 🙂 |
I agree. What I meant was making the |
|
To summarize it looks like in general we have some consensus that the work here is good with the following observations:
For 1 above, I suggest we use the limited scope of this PR to see how this works with the Query Loop block variations and move the larger conversation around expanding this to all block controls to a separate issue. This allows surfacing this narrower scope that meets some important use-cases in WP 6.1 (with the deadline looming near). For 2 since the typing issue already exists for the |
|
I'm adding my ✅ to this. This fits our use-case. I still have some concerns about the white-listing of controls vs. the black-listing approach offered in my alternative PR. Having to specifically white-list allowed controls, means that if the Core Query block is updated with new cool controls, people will have to specifically opt-in. And they might not know about them, as knowing whether a block has been updated with new control (especially if it's something global) might be hard. I think opting out of control is a better approach, that allows sensible defaults without having to think too much. Then devs can really decide if they don't want something and disable it. As for the rest of the comments and concerns, I agree they can be addressed in future PRs. |
There was a problem hiding this comment.
This looks good to merge with considerations outlined here.
I think opting out of control is a better approach, that allows sensible defaults without having to think too much. Then devs can really decide if they don't want something and disable it.
In some cases I think that may be true, but in the context of the Query Loop block and variations I think it's more likely the surprising appearance of additional controls could be undesirable. By opting into using the allowedControls API, this is a signal for curated control UIs exposed to the user. It's rather trivial for plugins to register additional allowedControls and then, release. End users would update to get that additional functionality. However, the opposite could be problematic if the additional functionality is undesirable as that would surface to the end user sooner.
|
I echo what @nerrad shared in the last comment. There are pros and cons to both approaches, but having a list of allowed controls prevents accidental exposure of unwanted UI options. |
|
Alright! I stand with your decision here, just wanted to voice my thoughts. |
|
Thanks all for the reviews and the feedback! This PR definitely surfaced some interesting issues we need to explore/address in the future. |
Detail and explain how to make best use of the changes introduced in the following PRs: WordPress#43590, WordPress#43632 and WordPress#44093
* Refactor Product Query to use the latest Gutenberg APIs As we worked with Gutenberg folks in WordPress/gutenberg#43590, WordPress/gutenberg#43632 and WordPress/gutenberg#44093 we have created a standard API that could be used for our use-case. This PR refactors our WIP experimental work to use that standardized API.
* Add docs for extending the Query Loop block via variations Detail and explain how to make best use of the changes introduced in the following PRs: #43590, #43632 and #44093 * Fix typo Change enhables to enables Co-authored-by: Sören Wrede <[email protected]> * Address code review feedback Specifically: * Added the complete code at the beginning * Change `isActive` from array to function * Reworded a few things * Added information about custom logic and other hints * Update docs/how-to-guides/block-tutorial/extending-the-query-loop-block.md * Change wording of opening paragraph for the example * Add section about innerBlocks and `scope` shortcomings * rename to `allowedControls` Co-authored-by: Sören Wrede <[email protected]> Co-authored-by: Ryan Welcher <[email protected]> Co-authored-by: Nik Tsekouras <[email protected]>
* Refactor Product Query to use the latest Gutenberg APIs As we worked with Gutenberg folks in WordPress/gutenberg#43590, WordPress/gutenberg#43632 and WordPress/gutenberg#44093 we have created a standard API that could be used for our use-case. This PR refactors our WIP experimental work to use that standardized API.
…7169) * Refactor Product Query to use the latest Gutenberg APIs As we worked with Gutenberg folks in WordPress/gutenberg#43590, WordPress/gutenberg#43632 and WordPress/gutenberg#44093 we have created a standard API that could be used for our use-case. This PR refactors our WIP experimental work to use that standardized API.
What?
Related: #40941
We need to find a good way to allow third party devs to extent Query Loop block easily and in a consistent way. I don't think(at least for now) that we should add a new general API for block variations. Instead, since Query Loop is a quite complicated block and is quite hard to implement everything that is possible(imagine post meta and their UI handling in a generic way) we could have explicit complex handling of this cases in the block itself.
In this PR I'm proposing and an
allowedControlsproperty in variations that should handle the hiding/showing of controls. While it's more verbose than ahideControlsit will be more flexible for extenders if some new control is added to Query Loop block.In general I think we would need(notes for follow up PRs):
namespaceattribute to constrain any changes per variationquerywith their own logic(mostly for meta in my mind for now)build_query_vars_from_query_block#43590)Testing Instructions
Products Listblock(added in this PR for testing purposes)Example
Products ListvariationAlternatively test with your variations.
Notes
While exploring the client side preview issues, I realized that the REST API doesn't support
meta_query. This is a problem of REST API and the way devs do it now, is by adding PHP filters which check for extra custom request params in order to update the query..What is missing and not mentioned? 🤔
--cc @sunyatasattva @gigitux