Curate queries within a block #530
Conversation
… add/multiple-queries-to-single-block
… add/multiple-queries-to-single-block
ingeniumed
changed the title
…aving the query selection as it's own component. Query entry logic is broken
…eing reset on a reload
…ontainer code, its been replaced rn
selector query within the standard happy path. All the inline-block binding code has been commented out as that's not supported at the moment.
…/remote-data-blocks into add/multiple-queries-to-single-block
|
I've re-done the entire config system based on the feedback provided by @chriszarate. The new config format looks as follows: register_remote_data_block( [
'title' => 'Art Institute of Chicago',
'icon' => 'art',
'queries' => [
// Changing the name of this query to anything but display will break existing blocks content.
'display' => $get_art_query,
'search_art' => $search_art_query,
],
'placeholders' => [
[
'name' => 'Get Art',
'query_key' => 'display',
],
],
] );I've kept I've also updated the example blocks to use the new config format. I went back and forth for this, as that would reduce the amount of changes in this PR. But, it's better to just isolate all the changes in this PR then. Lemme know if it's better to move this to another PR as I'm happy to do that. Items I've punted to a future PR that I won't tackle in this PR:
None of these are critical to the merging of this PR and feature imo. |
| return ( | ||
| <> | ||
| <div { ...blockProps }> | ||
| <EditErrorBoundary blockTitle={ getBlockTitle( props.blockName ) }> |
There was a problem hiding this comment.
Is having an ErrorBoundary here, and in the Edit component the right way to handle errors from each component? At the top level, the error from inside QueryComponent wasn't handled, but it was when it was split into two like this. Realistically, does Edit even need an error boundary now? I kept it to be safe but I feel like it could be skipped.
| ), | ||
| ] ), | ||
| 'selection_queries' => Types::nullable( | ||
| 'placeholders' => Types::nullable( |
There was a problem hiding this comment.
Nitpick: I might lean towards placeholder (singular) since it refers to the UI component, not the buttons / actions it contains.
There was a problem hiding this comment.
On one hand, I get where you are coming from since the UI itself is just 1 block. But on the other hand, it feels weird to call it a singular placeholder when several buttons are showing and each of those buttons corresponds to an entirely different query. With the support for multiple display queries, and multiple schemas it's likely even the UI would be different once the query is picked.
| query_key: string; | ||
| type: string; | ||
| }[]; | ||
| patterns: Record< string, string >; |
There was a problem hiding this comment.
In order to allow for multiple schemas to be mapped to the block bindings for each query, the patterns had to be differentiated as well. So I store the query_key in the keywords, and then map that name to the pattern. Since the query_key is dynamic, the types had to be widened.
There was a problem hiding this comment.
Since we are allowing multiple schemas to be mapped to a single block (via distinct "display" query keys), I think the block bindings need to store the query key in their source args. That way we know if they are compatible by inspecting the binding (instead of via manually managed keywords). We can enhance the existing hasBlockBinding util for this purpose.
There was a problem hiding this comment.
Let me explain what I did a bit more, because we might be talking about two different areas:
-
Patterns: This is meant to register different patterns for each display query. Since each query could have a different schema, it's important to differentiate the patterns. So each pattern has the display query stored as the keyword when registering. Then, the patterns are mapped using that query key in
REMOTE_DATA_BLOCKS. So when trying to show the pattern selection screen, the correct pattern is picked out fromREMOTE_DATA_BLOCKSand then selected from the pattern store in the block editor. I have a gif up top showing two art institute queries with this setup that would help make that clear. -
Block bindings: This is meant to register different block bindings per query, based on their schema. This is purely for building
REMOTE_DATA_BLOCKSand shares the code with the pattern registration. In the UI code, this map is useful for performing CRUD operations on the binding based on the query the block is using. This would be the UI sidebar where we pick the fields (can change based on the query's schema) for example.
Hopefully I haven't misunderstood what you mean. The main goal was to translate the user provided block/query config into information that the client could use to actually build out the new placeholder UI feature.
There was a problem hiding this comment.
Patterns: This is meant to register different patterns for each display query.
Understood here, all that makes sense. But instead of using keywords to maintain a correspondence between patterns and display queries, we should store the query key in the block binding source args. That way the correspondence is data-driven and between the things that actually have the relationship (bindings and query keys). We can use / improve a fn like hasBlockBinding to determine if a pattern corresponds to a display query by inspecting the bindings inside of the pattern.
It will also help us highlight (in the UI) situations where bindings "wander" or the display query is changed. In those situations, in the future, we can let the user know that their binding is broken.
Block bindings: This is meant to register different block bindings per query, based on their schema. This is purely for building REMOTE_DATA_BLOCKS and shares the code with the pattern registration. In the UI code, this map is useful for performing CRUD operations on the binding based on the query the block is using.
I think I'm lost here. There is still only one block binding source that is registered. Its behavior is controlled by the ancestor block context (remoteData attribute) and the binding source args.
There was a problem hiding this comment.
I like the idea, as it'll add more inference across the code rather than having the server send out the config to use.
That said, I don't think this change should be done in this PR. I'd push to punt that to another one. This PR has been open for a bit, it's big enough in its scope and works well enough that I'd like to push off any more changes unless they are really needed. IMO, the proposed changes aren't needed in this PR (note: not saying not at all, just in this PR).
They are part of the future changes stemming from a large number of areas that have been revealed to work the way they do because we assumed 1 query key called display would be used. While, I have attempted to change that as much as possible or to the extent possible while keeping the PR from blowing there's a lot still to be done. It's not new features, just QOL improvements imo.
Description
This PR is meant to be a continuation of #405.
The goal is to allow grouping multiple queries within a block, in the block editor. It'll allow query curation based on the data source within the block editor. So as an example, a couple of queries targetting the products in your shopify site could be grouped in the shopify block saving you the time of inserting these individually.
As part of this the schema was changed:
New Schema Changes
Block Registration
queries: This is the list of queries to be registered. There's no need to separate them or to mention their linkage.display_queries: The queries that are display queries from the list of queries provided.A big big shoutout to @brookewp as her work in #405 is what I'm building on 🙏🏾
In addition:
Screenshots
How existing blocks that aren't updated look like:
How new blocks that use multiple display queries look like:
The config object for this block:
Video Demonstration
Note that the first block doesn't have the artist title while the second block does. It's meant to highlight different outputs schemas for the two display queries.