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 Ignore option to sticky options for Query Loop block #66222

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

Conversation

coreyworrell
Copy link
Contributor

@coreyworrell coreyworrell commented Oct 17, 2024

What?

Query block sticky posts needs "Ignore" option

See #66221

Why?

Currently there are options:

Include: default behavior where sticky posts are prepended to returned posts
Exclude: all sticky posts are excluded
Only: only sticky posts are included

There should be a final option:

Ignore: sticky posts are included in their natural order (basically as if they were not sticky)

This would simply set 'ignore_sticky_posts' => true in the query.

This is very useful for building a "Recent posts" list where we just want most recent regardless of sticky or not.

How?

It adds an Ignore option for user to select and then sets the query var to ignore sticky posts.

Testing Instructions

  1. Create a new post, or set existing post as Sticky
  2. Edit a page or template
  3. Insert a Query Loop block
  4. Uncheck Inherit query from template
  5. For Sticky Posts, select Ignore option
  6. See sticky posts in natural sorting order

Testing Instructions for Keyboard

Screenshots or screencast

Copy link

github-actions bot commented Oct 17, 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.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: coreyworrell <[email protected]>
Co-authored-by: draganescu <[email protected]>
Co-authored-by: carolinan <[email protected]>
Co-authored-by: ntsekouras <[email protected]>
Co-authored-by: jameskoster <[email protected]>

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

@coreyworrell
Copy link
Contributor Author

@carolinan
Copy link
Contributor

Hi @coreyworrell Please complete the sections in the pull request description: Why, how, and testing instructions.

You need to to add a temporary compatibility function to Gutenberg that updates build_query_vars_from_query_block.
It is not enough to only update it in WordPress core since that would prevent the feature from being tested without the development version of WordPress.

Please see the examples in https://github.com/WordPress/gutenberg/tree/trunk/lib/compat/wordpress-6.7

@carolinan carolinan added [Type] Enhancement A suggestion for improvement. [Block] Query Loop Affects the Query Loop Block Needs PHP backport Needs PHP backport to Core labels Oct 23, 2024
@coreyworrell
Copy link
Contributor Author

@carolinan Thank you, I have updated the description and added the compat code just now.

@carolinan
Copy link
Contributor

The changes work well in my test.

To me it makes sense to support ignore_sticky_posts in the query loop block but a final decision needs to be made. @WordPress/gutenberg-core
I don't recall why it was not included when the other sticky post options were added.

I would like to make the following change requests:
The help text "Sticky posts always appear first, regardless of their publish date." is not accurate when this option is selected, and needs to be adjusted conditionally.
The coding standard errors need to be fixed and the backporting changelog needs to be added.
The PHP change needs to be added to a 6.8 compatibility folder, since no more enhancements can be made to 6.7.

Copy link
Contributor

@draganescu draganescu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the idea here is good. Left some comments as to the state of the PR.

// core function.
$sticky = get_option( 'sticky_posts' );

$query['post__not_in'] = array_diff( $query['post__not_in'], ! empty( $sticky ) ? $sticky : array() );
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we exlude sticky posts? Ignore set below should include them and just list them in their natural order, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Line 125 doesn't exclude them, it's actually re-including them after core already excluded them. That's what the comment above line 123 is about.


/**
* Adds `ignore` option for sticky posts to the Query block.
*
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The linting has to be addressed.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed now

@@ -8,6 +8,7 @@ const stickyOptions = [
{ label: __( 'Include' ), value: '' },
{ label: __( 'Exclude' ), value: 'exclude' },
{ label: __( 'Only' ), value: 'only' },
{ label: __( 'Ignore' ), value: 'ignore' },
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How does this affect the editor? Shouldn't there be some change to the querying done clientside in the editor too?

Copy link
Contributor

@carolinan carolinan Oct 28, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great question.

In the editor, the sticky posts are already bugged on trunk. For example sticky posts do not show as the first posts when the "include" option is set, which is the default. The result in the editor and front already does not match.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should at least try to see if we can match the preview in editor. As @carolinan says it's already buggy and if I remember correctly is about what REST API supports how it maps with WP_Query. That's probably the reason (the mapping) for not having this as a separate option for sticky control.

@coreyworrell
Copy link
Contributor Author

coreyworrell commented Oct 29, 2024

Edit: I was able to add the backport

@carolinan

The coding standard errors need to be fixed and the backporting changelog needs to be added. The PHP change needs to be added to a 6.8 compatibility folder, since no more enhancements can be made to 6.7.

Are you able to help with the backporting changelog, I'm not familiar with that process? Thanks very much.

@ntsekouras
Copy link
Contributor

Thanks for the PR! It's been a while, but I think the reason for not having this option initially was about what REST API supports how it maps with WP_Query. It might just have also been an oversight.

I did some testing and it seemed to work well, but this block has so many nuances and we need to test it really well. We need to do good testing with pagination too, because if I remember correctly there are quirks about it. We also need to test existing blocks that remain unaffected.

@@ -8,6 +8,7 @@ const stickyOptions = [
{ label: __( 'Include' ), value: '' },
{ label: __( 'Exclude' ), value: 'exclude' },
{ label: __( 'Only' ), value: 'only' },
{ label: __( 'Ignore' ), value: 'ignore' },
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ignore label is not self explanatory in this context and is quite similar to the exclude, where it's ignore post stickiness.

Maybe a different control can be used with help texts? --cc @jameskoster

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps 'Ignore stickiness' is okay for now?

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 Needs PHP backport Needs PHP backport to Core [Type] Enhancement A suggestion for improvement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants