-
Notifications
You must be signed in to change notification settings - Fork 2
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
Issue-142: Add option to pull Trending posts #143
Issue-142: Add option to pull Trending posts #143
Conversation
…add-option-pull-trending-posts
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.
High-level it's looking good. I've got some questions on the details.
} | ||
if ( empty( $posts ) ) { | ||
$query = new \WP_Query( $args ); | ||
$posts = $query->posts; |
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.
Am I following that the "correct" results of this endpoint would be the same results that are going to appear on the frontend for the block? Would we want to use the same Post_Queries
instance that are used when filtering the block output here instead of a plain WP_Query
?
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.
@dlh01 the "correct" results will not have any pinned posts or deduplication. That all happens client side in the editor, so it can be just a plain WP_Query
or the Post_Queries
equivalent of a plain WP_Query
. We do want to support the trending override, of course. So we could do just Trending_Post_Queries
and Variable_Post_Queries
.
/** | ||
* Pull trending posts from Parsely. | ||
*/ | ||
final class Trending_Post_Queries implements Post_Queries { |
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.
If this is specific to Parse.ly, then it should be named as such. If it's intended to be a generic trending posts class, then it should be structured so that it supports multiple providers, of which Parse.ly will be the first (and we could add other providers like GA4 later).
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.
@kevinfodness supporting multiple providers was my plan... I kind of figured we would sort out how structure it when we added the second provider.
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.
Ah, we're using named arguments instead of positional ones, this works for me!
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.
Looks good but feel free to keep getting others feedback on this if you need to 🐙
Description
The backfill should allow pulling featured posts from Parse.ly and eventually lede-beacon. We have now added the functionality to also pull trending posts, enhancing the variety of content that can be used for backfill. Editor controls have been set up to facilitate the selection of these posts, and a new endpoint has been developed to handle these queries effectively.
Use Case
A user can select trending posts for the backfill source, in addition to the previously available options. This expansion allows for a more dynamic content selection process.
Issue
Add option to pull Trending posts - #142