[Gutenberg] Search block for Homepage #18
Conversation
miina
changed the title
miina
changed the title
miina
changed the title
functions.php
Outdated
| function travel_filter_search_pre_get_posts( $query ) { | ||
|
|
||
| if ( $query->is_search ) { | ||
| $query->set( 'post_type', array( 'post', 'adventure' ) ); |
There was a problem hiding this comment.
Wondering if this should search from adventure post type only. Thought that perhaps for now it can search from blog posts as well and changed later if needed. Thoughts?
miina
changed the title
miina
changed the title
There was a problem hiding this comment.
Looks Good, A Few Points
Hi @miina,
As you mentioned, this is dependent on Issue #7 (PR #33). And maybe you didn't intend for this to be reviewed yet. I just made these points because the parent issue (#26) is "Ready For Review."
This PR now shows a diff of 38 files, but this diff of the PR's branch feature/search_block to develop shows only 4 files. Maybe it didn't recompute the diff after merging #13. But I only reviewed that smaller diff.
includes/class-amp-travel-blocks.php
Outdated
| * | ||
| * @param object $query Original query. | ||
| */ | ||
| public function travel_filter_search_pre_get_posts( $query ) { |
There was a problem hiding this comment.
It looks like this method name might need to be filter_search_pre_get_posts to match the add_filter() call above:
add_filter( 'pre_get_posts', array( $this, 'filter_search_pre_get_posts' ), 10, 1 );There was a problem hiding this comment.
Thanks for catching this!
assets/css/editor-blocks.css
Outdated
| .travel-search .travel-link { | ||
| color: #8b58e3 | ||
| } | ||
| .travel-input-group { |
There was a problem hiding this comment.
Could you please add an empty line above this selector? And similarly for the 2 selectors above this?
Otherwise, it'd be fine to remove the empty line above .travel-search-input.blocks-rich-text.
includes/class-amp-travel-blocks.php
Outdated
| /** | ||
| * Modify the default search query to include 'adventure' post type. | ||
| * | ||
| * @param object $query Original query. |
There was a problem hiding this comment.
Could you please remove the extra space between $query and Original query. It's hard to see, but there are 2 spaces.
includes/class-amp-travel-blocks.php
Outdated
| add_action( 'enqueue_block_editor_assets', array( $this, 'enqueue_editor_scripts' ) ); | ||
| add_filter( 'the_content', array( $this, 'filter_the_content_amp_atts' ), 10, 1 ); | ||
| add_filter( 'wp_kses_allowed_html', array( $this, 'filter_wp_kses_allowed_html' ), 10, 2 ); | ||
| add_filter( 'pre_get_posts', array( $this, 'filter_search_pre_get_posts' ), 10, 1 ); |
There was a problem hiding this comment.
This should probably be add_action( 'pre_get_posts', instead of add_filter.
Also, the 3rd argument of 1 doesn't hurt, but this would probably be cleaner without it.
|
@kienstra Thank you for reviewing. Made the changes + also added a placeholder for |
In most cases, retain both edits. Also, change add_filter( 'pre_get_posts' ) to add_action( 'pre_get_posts' ).
|
Approved Hi @miina, Thanks for adding the search.php template. As you mentioned, it's a placeholder. So I didn't review it much. |
This PR is based on #13 .Fixes #26.
The PR aims to do the following:
Missing dependencies
Blocked by #7.