-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Query block: Allow inheriting the global query arguments #27128
Conversation
Size Change: +119 B (0%) Total Size: 1.21 MB
ℹ️ View Unchanged
|
@ntsekouras I started working on a proof of concept for this one, but there's still some things that need discussing and some decisions: Default valueShould the setting default to custom query, or global query? If we default to the global query, then perhaps we'll need to write a migration/deprecation script so that existing FSE themes don't break. On the other hand we may not need that considering the experimental nature of FSE themes and the fact that we have a warning in their dashboard that says
So... breaking changes are to be expected. What should we show in the preview?The simple implementation I pushed here doesn't change what the preview shows. So users can change the preview context by switching the checkbox and tweaking the arguments of the query. That will change the preview, and when they switch to the global query the arguments they chose are still used for the preview.
Opinions? |
Hey Ari 👋 - I really appreciate you're working on this! This is something I have explored myself without a clear solution yet. I think my main obstacle is this:
This is the main problem to me and changes in FSE navigation and previews make it even harder... Maybe this will help about the context detection more #27124 ..?
Actually I think this is discussed and probably will end up like that.
That is not a bad option, yes. I'd like to try it out though to see how it feels. I would love some other opinions on this as well.
I believe it should default to In my mind this message could be like 'You are in We will still need the option though for changing after creation..
IMO this is not necessary now since there are almost always breaking changes here, is experimental and the block themes are so few for now. However these changes are needed as Query block now matures and evolves.. |
I think this could work well and I'd like to try it!
|
The preview idea of Ari :#27128 (comment) seems promising and with some design, it could probably serve us well. |
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 will need the design but I'm very happy with this going forward! Great idea Ari 💯
@@ -26,8 +26,18 @@ function render_block_core_query_loop( $attributes, $content, $block ) { | |||
'post__not_in' => array(), | |||
); | |||
|
|||
$use_global_query = ( isset( $block->context['query']['customQuery'] ) && false === $block->context['query']['customQuery'] ); |
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 the property could be renamed to useGlobalQuery
and therefore invert the logic/checks in code. Also this should be a top level property of Query
block (changes in block.json etc..)
packages/block-library/src/query/edit/query-inspector-controls.js
Outdated
Show resolved
Hide resolved
Help me understand what the purpose of the preview is? While there are some aspects of the controls, how they stack in the sidebar, the verbiage and using a toggle instead of a checkbox, having the help text be contextual, generally this toggle seems to make sense to me: It tells me that when it's off, the inherited query affects the loop. But that when it's toggled, it's the query fields below, and not the global query, that affects the loop. Is that correct? |
"Preview" may have been the wrong word to use, but since we don't have the global
Correct. The verbiage and UX does need to be improved, this was just a proof of concept to figure out what we can do. But as a concept I think it can work - as long as we figure out the verbiage etc to convey that the query arguments are so that they can preview their block on different contexts 🤔 |
Oh, thanks for explaining! But the feature is still primarily meant to act as a mechanism to customize the query on a per template basis, right? And the preview aspect is an additional feature on top? Permit me to walk through this:
That seems to be the primary use case, please correct me if I'm wrong. The ability to change the content that gets loaded into a template is definitely a legitimate use case. Perhaps the theme has only an Index template, and if you want to customize the date archive or the search results, it's fair for you to want to be able to see that. However my instinct is that this act of previewing needs better real estate than the query toggle in the sidebar. The feature should probably affect the whole page, and since it is conceptually related to the template hierarchy, could even be a primary action. I'd appreciate thoughts by @jameskoster as I know he's worked on related things — could this have a place in the same template dropdown dialog in the top center of the screen? Or should it indeed be a choice in the Preview dropdown menu? |
I'm wrapping up the day, but I had this thought that maybe we could try using the new experimental API for block variations transforms, so it could work this way:
|
One downside of using preview mode that I can think of (on the technical level) with this approach is that attributes that are modified are stored inside the saved content even when they aren't used during rendering. |
The relationship between template and query are closely linked, which makes me a little apprehensive about adding this customisation option. It may be that I'm missing something obvious though. Rough example – should the UI really allow someone to change their Allowing some controls to be customised might make sense though, like the number of entries to display. But even that should probably be governed somewhat by the template. In the same example as before, it wouldn't make much sense to change the number of entries displayed on a
Agreed on this. I know this PR is only a draft, but I'd also add that there's something awkward about the UX of choosing a preview in the block settings. I would be tempted to consider splitting this in to two issues and exploring accordingly:
|
The main problem we have with inheriting the global query is that we don't know what to show in the editor (what I call "preview" above). Assume for example that the theme only has an
Correct. But that is not necessarily a downside... In the example above where the query block is used inside a
Right now there is no global-query inheritance. So without the global query, there is no such thing as a semantic template anyway... There are only custom queries everywhere and they are not URL-aware. This PR adds a way to make them semantic and not all queries are forced to be custom queries (which is their current state). |
Much appreciate your further explanation here. To me, this only reaffirms that this deserves better real-estate than being attached to just a single block that may or not (🙈) be present in the template. I wonder if a path forward is to help land #25309 and see if that preview dropdown can be a place to configure the query preview? It feels an obvious place to integrate it, it's global to the whole page, it's query block agnostic, and it affords a place to provide helper text to explain the limitations. |
I've been going back and forth in my mind, trying to figure out the minimum viable implementation we can have for this, so we can start moving it forward, and polish as we go.
With the above in mind, I tested manually creating a CPT using this code: (pasting here to make testing easier) add_action( 'init', function() {
register_post_type(
'recipe',
[
'labels' => [
'name' => _x( 'Recipes', 'Post type general name', 'recipe' ),
'singular_name' => _x( 'Recipe', 'Post type singular name', 'recipe' ),
'menu_name' => _x( 'Recipes', 'Admin Menu text', 'recipe' ),
'name_admin_bar' => _x( 'Recipe', 'Add New on Toolbar', 'recipe' ),
'add_new' => __( 'Add New', 'recipe' ),
'add_new_item' => __( 'Add New recipe', 'recipe' ),
'new_item' => __( 'New recipe', 'recipe' ),
'edit_item' => __( 'Edit recipe', 'recipe' ),
'view_item' => __( 'View recipe', 'recipe' ),
'all_items' => __( 'All recipes', 'recipe' ),
'search_items' => __( 'Search recipes', 'recipe' ),
'parent_item_colon' => __( 'Parent recipes:', 'recipe' ),
'not_found' => __( 'No recipes found.', 'recipe' ),
'not_found_in_trash' => __( 'No recipes found in Trash.', 'recipe' ),
'featured_image' => _x( 'Recipe Cover Image', 'Overrides the “Featured Image” phrase for this post type. Added in 4.3', 'recipe' ),
'set_featured_image' => _x( 'Set cover image', 'Overrides the “Set featured image” phrase for this post type. Added in 4.3', 'recipe' ),
'remove_featured_image' => _x( 'Remove cover image', 'Overrides the “Remove featured image” phrase for this post type. Added in 4.3', 'recipe' ),
'use_featured_image' => _x( 'Use as cover image', 'Overrides the “Use as featured image” phrase for this post type. Added in 4.3', 'recipe' ),
'archives' => _x( 'Recipe archives', 'The post type archive label used in nav menus. Default “Post Archives”. Added in 4.4', 'recipe' ),
'insert_into_item' => _x( 'Insert into recipe', 'Overrides the “Insert into post”/”Insert into page” phrase (used when inserting media into a post). Added in 4.4', 'recipe' ),
'uploaded_to_this_item' => _x( 'Uploaded to this recipe', 'Overrides the “Uploaded to this post”/”Uploaded to this page” phrase (used when viewing media attached to a post). Added in 4.4', 'recipe' ),
'filter_items_list' => _x( 'Filter recipes list', 'Screen reader text for the filter links heading on the post type listing screen. Default “Filter posts list”/”Filter pages list”. Added in 4.4', 'recipe' ),
'items_list_navigation' => _x( 'Recipes list navigation', 'Screen reader text for the pagination heading on the post type listing screen. Default “Posts list navigation”/”Pages list navigation”. Added in 4.4', 'recipe' ),
'items_list' => _x( 'Recipes list', 'Screen reader text for the items list heading on the post type listing screen. Default “Posts list”/”Pages list”. Added in 4.4', 'recipe' ),
],
'description' => 'Recipe custom post type.',
'public' => true,
'publicly_queryable' => true,
'show_ui' => true,
'show_in_menu' => true,
'query_var' => true,
'rewrite' => array( 'slug' => 'recipe' ),
'capability_type' => 'post',
'has_archive' => true,
'hierarchical' => false,
'menu_position' => 20,
'supports' => array( 'title', 'editor', 'author', 'thumbnail' ),
'taxonomies' => array( 'category', 'post_tag' ),
'show_in_rest' => true
]
);
} ); I then created 2 sample posts in that CPT, and manually created from Appearance > Templates an Note1: I had to push an extra commit to make the query in site-editor work for the post-type. So... the question now is this: Should we merge a change that works and covers everything we need now? Or should we work on things and try to make the context changes for taxonomies etc - even if they are not vital at this point? We can always work on them after an initial implementation gets merged IMO, and keep polishing it. |
Is this documented as a requirement on the Site Editor? BTW. Where you used |
No, these are not requirements. They are not documented. I assume the reason they are not documented is because the UI is still in flux. There is no way to create these from the site-editor yet. Eventually I'm sure it will be possible, but not at this point. So there's no reason to document something that is not yet finalized. Right now, you can not create a category-archive from the site-editor, because the UI for those has not been built yet. But you can create them from Appearance > Templates. Eventually those 2 will probably be merged in some fashion, so Appearance > Templates will use the site-editor and we'll be able to do everything there (emphasis on the eventually part of the sentence)
Yes, I only wrote it like |
+1 for merging a minimum-viable-implementation. |
To sum up the posts above and what I've been thinking lately: |
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 haven't tested with the latest changes but it's something we can land with the remark I left in my comment. It isn't ideal but it isn't simple to solve. Wr shouldn't block this functionality for the sake of perfection though.
Description
Fixes #25377
Related: #25790.
Adds a setting to customize the query, or inherit the global query.
What needs to be done:
Write deprecation script so for existing query blocks the new setting defaults to custom-query instead of globalTypes of changes
Checklist: