[FEATURE] Custom Faceted Directory - #376
Conversation
GeoffDusome
added
Needs FE Review
Needs BE Review
and removed
needs work
DO NOT MERGE
labels
…om:moderntribe/ModernPress into feature/MOOSE-350/faceted-directory-custom
MlKilderkin
left a comment
MlKilderkin
left a comment
There was a problem hiding this comment.
Could we split this into smaller PRs next time? Currently it is very hard to review
| $this->request = is_array( $request ) ? $request : []; | ||
| $this->paged = max( 1, absint( $this->request[ Facet_Registry::PAGE_PARAM ] ?? get_query_var( 'paged' ) ?: 1 ) ); | ||
|
|
||
| $built = tribe_project()->container()->get( Directory_Query::class )->build( |
There was a problem hiding this comment.
I am curios why we do in such way and don't actually do DI in constructor? public function __construct( Directory_Query $dir_query, array $args = [] )
There was a problem hiding this comment.
I would assume it's just a style choice. I don't think there's a correct way to do this right?
| : 'top'; | ||
|
|
||
| $post_types = $this->attributes['postTypes'] ?? [ 'post' ]; | ||
| $this->post_types = Facet_Registry::filter_public_post_types( |
There was a problem hiding this comment.
Could we use DI in controller instead of static method call? Same applies to other places
There was a problem hiding this comment.
I don't really understand the harm in the way this is set up, if you feel strongly please feel free to make a pass at the PR.
| * One row per post + facet + term, so a multi-facet filter resolves in a single | ||
| * grouped query instead of one taxonomy join per facet. | ||
| */ | ||
| class Facet_Index { |
There was a problem hiding this comment.
Do we have kind of benchmark for this how long does it takes to rebuild the index? For example: Kinsta hosting likely fail you if your job will be run more than 30 seconds. Pantheon as well. We might need to do that in batches
There was a problem hiding this comment.
I have no benchmarks currently and have no real way to run any. This is a good call. I think it makes sense to look into this in a follow-up or maybe on a project that would be on a hosting platform that would require it.
| * | ||
| * @return array<string, bool> | ||
| */ | ||
| private static function add_ancestors( array $keep, array $terms ): array { |
There was a problem hiding this comment.
Generally curios why we(AI) keep trying to use static methods? Almost all architecture is built on usage of DI and only s1 ported legacy uses static
There was a problem hiding this comment.
I believe the idea is that the static methods have no dependency on the class itself so there's no need to use DI. I don't think static methods need to leave our codebase entirely, but maybe we need to change the location of them to a Helpers class or something instead?
Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
…rite newer ones Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
…args Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
|
@dpellenwood - I'd like to give BE a chance to actually go through this work and adjust to what they think makes the most sense. Because this was written mostly by AI, it's entirely possible things can be adjusted, reordered, split out into different files, or rewritten differently (and better). Myk had some good points in his review, but I don't know that all of it needs to be addressed right away. Typically if we had to do something like this on a project a BE developer would have the BE created prior to me touching the FE, so we're working a bit backwards here. Personally, I would feel more comfortable with this if BE has a chance to actually go hands on with it to make it our own int he way we'd do it. If we could get Myk (or anyone else) some time to work on this that would be great. Because this feature is so large and important, I think it's important that we do our due diligence and do this the right way. |
2 participants
What does this do/fix?
This pull request introduces several new block controllers for faceted directory functionality, updates the block registration, and improves context handling for block rendering. The most important changes are outlined below.
Faceted Directory Feature Implementation:
Directory_Grid_Controller,Faceted_Directory_Controller, andFilter_Bar_Controllerin theComponents/Blocksnamespace, providing the core logic for faceted directories, directory grid display, and filter bar controls. These controllers handle pagination, post type filtering, facet rendering, and block context management. [1] [2] [3]Block Registration and Core Integration:
tribe/directory-grid,tribe/faceted-directory,tribe/filter-bar) in theBlocks_Definerso they are available in the editor and frontend.Coreclass, ensuring the new faceted directory features are loaded and hooked into the plugin lifecycle. [1] [2]Block Controller Improvements:
Abstract_Block_Controllerto support richer block context handling, including a fallback mechanism for editor preview context and a flag to control this behavior. This ensures consistent context data for both frontend and editor previews. [1] [2]Media Handling Enhancement:
get_mediamethod in thePost_Datatrait to return a placeholder image when no media is available, improving the user experience for posts without featured images.Changelog Update:
QA
Links to relevant issues
Screenshots/video:
Demo Environment:
Pull request checklist