-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
Add option to exclude current post from query block #64916
base: trunk
Are you sure you want to change the base?
Add option to exclude current post from query block #64916
Conversation
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 Unlinked AccountsThe following contributors have not linked their GitHub and WordPress.org accounts: @noellesteegs, @chrisgresh, @supernovia. Contributors, please read how to link your accounts to ensure your work is properly credited in WordPress releases. 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. |
👋 Thanks for your first Pull Request and for helping build the future of Gutenberg and WordPress, @g-elwell! In case you missed it, we'd love to have you join us in our Slack community. If you want to learn more about WordPress development in general, check out the Core Handbook full of helpful information. |
packages/block-library/package.json
Outdated
@@ -45,6 +45,7 @@ | |||
"@wordpress/date": "file:../date", | |||
"@wordpress/deprecated": "file:../deprecated", | |||
"@wordpress/dom": "file:../dom", | |||
"@wordpress/editor": "file:../editor", |
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.
👋🏻 There are a bunch of warnings suggesting that @wordpress/block-library should not depend on @wordpress/editor as blocks can be loaded into a non-post block editor.
In other places, to avoid declaring @wordpress/editor as a dependency, the store is accessed using the key string, e.g., select( 'core/editor' )
.
Example:
// FIXME: @wordpress/server-side-render should not depend on @wordpress/editor. |
Not sure how else to get the post's current id (aside from the URL params that is) 🤔
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.
Thanks for pointing this out! Having looked at some other instances where the key string was used I think it should be okay to do that here too.
I've added fallbacks to ensure this works in the site editor, or if the store isn't available at all.
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.
No, we shouldn't do that, even with a hardcoded string. We've tried to cleanup any remaining similar implementations and we should definitely not add more. Can you share the other instances you found? We should see to remove them.
The approach in such cases is to see how we can pass down the information needed. For this case I think the postId
from block's context is already there.
I just want to clarify how this works in the site editor. There are instances where we may want to exclude the current post from a template - ie when working on the single post template. In this situation we don't know the post ID, so nothing can/should be done in the preview, but the attribute we are setting will enable us to hide the current post on the frontend, when we have that context. Demo: Screen.Capture.on.2024-08-30.at.17-05-22.mov |
return $query; | ||
} | ||
add_filter( 'query_loop_block_query_vars', 'filter_query_block_exclude_current', 10, 2 ); | ||
} |
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.
Temporary changes to Gutenberg, that will be removed once the change is made in core and the plugin only supports that WordPress version or newer, should be added to the lib/compat/version folder.
You can find examples of this for reference in the wordpress-6.7 folder.
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.
Once approved, a Trac ticket and pull request needs to be created, referencing the Gutenberg PR, and a changelog entry needs to be added:
https://github.com/WordPress/gutenberg/tree/trunk/backport-changelog
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.
@carolinan is right about the location of such code. Additionally I don't think the filter would work in this case, because Query Loop internally can also set the post_not_in
value (check here).
So we would probably need to override this function in GB. It's been a while that we had changes there, but I had implemented some code in the webpack config to use build_query_vars_from_query_block
with gutenberg
prefix if available. We don't currently have an override for this function in GB trunk.
Check this for webpack.
Cool feature, I'm here for it. UI wise I wonder if we should use the same interface that "Sticky posts" do: This isn't because there are three options here, though could there be? Would it make sense to have a query loop that include only the current post? I can't think of a good reason. But mainly, it would potentially offer some consistency: The toggle can work too, but we need to tweak the help text to avoid the typographic widow. If you keep the toggle, perhaps the help text should be contextual. When toggled on: "The current post will be omitted". When toggled off, "The current post will be included." I don't love that, it's not super clear. But the feature seems valid enough, and it helps that these controls are hidden under the "Custom" toggle for the query. I'm also assuming we can omit this entire control for queries that don't have a "current post"? That seems an important aspect to settle. CC: @WordPress/gutenberg-design |
I don't know about this one, it seems a bit abstract / niche. There are situations where the option would be obsolete, for instance adding a custom query to a sidebar in a template like
This would help, but the conditions might be tricky to get right? I think it would help to include scenarios of when a user might want to use this feature. The 'Related posts' example above doesn't quite work for me; how are the posts deemed to be related? I have a hunch that a query variation with built in logic might work better. |
@jameskoster Twenty Twenty-Five is designed to have a section between the single post content and the footer, called "Other posts". |
And in classic themes "related posts" are normally done by applying the category or tag filter. I assumed that would be combined here as well. |
In past designs of mine, this has been quite common. On a homepage, I might show the latest post emphasized, and then subsequent posts in a table. Rich does something similar, to nice effect: https://rich.blog At the moment you can't accomplish this in a block theme without a slew of complex custom CSS and somewhat unsemantic markup as a result. It may be niche, but this is one of the areas where a custom query can actually make sense. It's not a strong opinion from my end, I think it can be acceptable to show the latest post, and then list an "archive" that duplicates that latest post in the list. But I thin the use case is definitely valid, so if a design could be found that struck the right balance, it could be useful. |
What if it is placed under filters so it is less prominent? |
That way perhaps at some point it could be combined with a filter that excluded specific post Id's.. |
I don't think that's possible currently? The single post template is not aware of the categories associated with the queried post. There is no UI for this.
How does this option enable that? It wouldn't help when the homepage is set to display latest posts, because there is no post to exclude in that context. When using a static homepage you'd use two loops with the second offsetting the query by one.
That sounds reasonable, but I'm still anxious about adding an option for this one narrow use case. If we go ahead then we should definitely hide it in contexts where it makes no sense, like those I mentioned above. |
The block is not limited to templates, a user can manually set the filters. |
But the use case you described is something you would add to the template, no? |
I don't think we can assume that the user will add it to a template and not in the block editor. |
I'm not saying we should. What I'm suggesting is that if we're going to add an option which only applies in certain scenarios, then it should only be visible to users in those scenarios. As a really basic example, if I add a Query Loop to my 404 template then I should not see an "Exclude current post" option because:
|
Ah yes that example is clear, thank you. There are multiple blocks that does not do anything depending on context. So what can be used to unblock this, an allow list of templates? |
That's a good question... an allow list seems like a good place to start, but there may be more nuance. That potential nuance is the main reason for my concern about this option. If the logic about when to display the option becomes convoluted that's an indicator that the solution may not be the right one. Another thing to check is how the option behaves in the context of an archive like the posts page, search results, or taxonomy archives: If a post contains a query that excludes, is the exclusion respected in archives where the Post Template is configured to display the full post content? |
I was under the impression that the use-case for this feature was well known, but if that's not the case, perhaps more input is needed from a variety of users. It's a very common pattern for a website to show "related" posts alongside, within, or below the content of a single post. Since the purpose of this is usually to direct users to view different content, it doesn't make sense to repeat the currently viewed post in this area. The main use-case would be within the single post template, but I could also imagine this being useful within post content too. There may not be a wide variety of other situations where this is needed, but the above problem is so common and so prominent that I think it warrants providing the option for users to resolve it. |
Don't get me wrong, you are indeed describing a common feature in general terms. But I'm not sure it's common in the context of the Query Loop block specifically; it has applications beyond the Single Post template, where the proposed setting makes no sense. As a provocation; should we consider a 'Related posts' variation of Query Loop? It could include exclusion logic behind the scenes and provide a UI where the user defines what 'related' means (shared categories, tags, etc), plus other options like how many posts to include. |
This sounds interesting, but I don't think I'm fully grasping the suggestion. Are there existing examples of variations where additional behaviour is introduced like this? I view variations as pre-configured blocks/innerblocks. Within a variation, how would the additional UI be presented to a user, and wouldn't it be confusing to offer a separate UI from the standard query block, opposed to making the core query block more flexible/powerful? I agree this setting makes no sense in certain contexts. I'm trying to avoid being biased, but I do think the use-case I described can't be understated. That said, I'm not tied to this solution if there's a better one available. |
Good question. From a cursory look at https://github.com/WordPress/gutenberg/blob/trunk/docs/how-to-guides/block-tutorial/extending-the-query-loop-block.md it seems that custom controls can be added / configured. To be clear, this was only a provocation :) I'm not opposed to adding the toggle, so long as we figure out the logic required to hide it in the UI when it makes no sense. |
What?
This PR adds the ability to exclude the current post from the query loop block
Fixes #42810
Why?
This is a commonly requested feature by users.
How?
This is achieved by adding a new attribute to the query block -
excludeCurrent
.This approach means we can avoid storing specific post IDs in block attributes, meaning if the query loop is copy/pasted between posts/pages or forms part of a pattern, it will behave appropriately.
A toggle control is rendered for this attribute in the block sidebar.
The new attribute is read both in the post template block JS and PHP query generation, to dynamically exclude the current post from the query within the block editor and on the frontend.
Note - the PHP side will require a change to WordPress core in the
build_query_vars_from_query_block
function, I've used a filter here to demonstrate the functionality.Testing Instructions
Testing Instructions for Keyboard
The additional UI component uses the existing
ToggleControl
so is keyboard accessible.Screenshots or screencast
Screen.Capture.on.2024-08-29.at.23-22-08.mov