Skip to content
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

Open
wants to merge 5 commits into
base: trunk
Choose a base branch
from

Conversation

g-elwell
Copy link

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

  1. Insert a query block into a post/page
  2. Toggle the "Exclude current post" option
  3. The current post should no longer be visible in the query block in the editor
  4. The current post should no longer be visible in the query block on the frontend

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

Copy link

github-actions bot commented Aug 29, 2024

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 props-bot label.

Unlinked Accounts

The 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.

Unlinked contributors: noellesteegs, chrisgresh, supernovia.

Co-authored-by: g-elwell <garethelwell@git.wordpress.org>
Co-authored-by: ramonjd <ramonopoly@git.wordpress.org>
Co-authored-by: carolinan <poena@git.wordpress.org>
Co-authored-by: jasmussen <joen@git.wordpress.org>
Co-authored-by: jameskoster <jameskoster@git.wordpress.org>
Co-authored-by: ntsekouras <ntsekouras@git.wordpress.org>
Co-authored-by: Marc-pi <mdxfr@git.wordpress.org>
Co-authored-by: mrfoxtalbot <mrfoxtalbot@git.wordpress.org>
Co-authored-by: carlomanf <manfcarlo@git.wordpress.org>
Co-authored-by: bbertucc <elblakeo31@git.wordpress.org>
Co-authored-by: dinhtungdu <dinhtungdu@git.wordpress.org>
Co-authored-by: lunule <lunule@git.wordpress.org>
Co-authored-by: efu98 <elfu98@git.wordpress.org>
Co-authored-by: richtabor <richtabor@git.wordpress.org>
Co-authored-by: Humanify-nl <humanify@git.wordpress.org>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

Copy link

👋 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.

@github-actions github-actions bot added the First-time Contributor Pull request opened by a first-time contributor to Gutenberg repository label Aug 29, 2024
@shail-mehta shail-mehta added [Type] Enhancement A suggestion for improvement. [Block] Query Loop Affects the Query Loop Block labels Aug 30, 2024
@@ -45,6 +45,7 @@
"@wordpress/date": "file:../date",
"@wordpress/deprecated": "file:../deprecated",
"@wordpress/dom": "file:../dom",
"@wordpress/editor": "file:../editor",
Copy link
Member

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) 🤔

Copy link
Author

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.

Copy link
Contributor

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.

@carolinan carolinan self-requested a review August 30, 2024 04:35
@g-elwell
Copy link
Author

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 );
}
Copy link
Contributor

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.

Copy link
Contributor

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

Copy link
Contributor

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.

@jasmussen
Copy link
Contributor

Cool feature, I'm here for it. UI wise I wonder if we should use the same interface that "Sticky posts" do:

Screenshot 2024-09-04 at 09 46 57

Screenshot 2024-09-04 at 09 46 52

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:

Screenshot 2024-09-04 at 09 49 15

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

@jameskoster
Copy link
Contributor

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 archive.html, or showing latest posts on 404.html.

I'm also assuming we can omit this entire control for queries that don't have a "current post"?

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.

@carolinan
Copy link
Contributor

carolinan commented Sep 4, 2024

@jameskoster Twenty Twenty-Five is designed to have a section between the single post content and the footer, called "Other posts".
Without being able to exclude the post, it's title and date will be shown twice. Once for the content and once in the section.
This is not uncommon, it is frequently requested, as seen in the issue.

@carolinan
Copy link
Contributor

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.

@jasmussen
Copy link
Contributor

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.

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.

@carolinan
Copy link
Contributor

What if it is placed under filters so it is less prominent?

@carolinan
Copy link
Contributor

That way perhaps at some point it could be combined with a filter that excluded specific post Id's..

@jameskoster
Copy link
Contributor

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.

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.

On a homepage, I might show the latest post emphasized, and then subsequent posts in a table

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.

Twenty Twenty-Five is designed to have a section between the single post content and the footer, called "Other posts".

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.

@carolinan
Copy link
Contributor

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.

The block is not limited to templates, a user can manually set the filters.

@jameskoster
Copy link
Contributor

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?

@carolinan
Copy link
Contributor

I don't think we can assume that the user will add it to a template and not in the block editor.

@jameskoster
Copy link
Contributor

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:

  1. It makes no sense
  2. It won't do anything

@carolinan
Copy link
Contributor

Ah yes that example is clear, thank you.

There are multiple blocks that does not do anything depending on context.
A featured image block, a query title block, a post content block etc also do not work on the 404.
These pre-existing scenarios could be used as motivation for including this setting; but I do want these to be fixed because yes, they are confusing.

So what can be used to unblock this, an allow list of templates?

@jameskoster
Copy link
Contributor

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?

@g-elwell
Copy link
Author

g-elwell commented Sep 6, 2024

I don't know about this one, it seems a bit abstract / niche.

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.

@jameskoster
Copy link
Contributor

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.

@g-elwell
Copy link
Author

g-elwell commented Sep 6, 2024

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.

@jameskoster
Copy link
Contributor

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Block] Query Loop Affects the Query Loop Block First-time Contributor Pull request opened by a first-time contributor to Gutenberg repository [Type] Enhancement A suggestion for improvement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Query loop - remove current post ID for ‘related posts’
7 participants