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

Query and Search blocks: support for Instant Search #63147

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

Conversation

r-chrzan
Copy link

@r-chrzan r-chrzan commented Jul 4, 2024

What?

Initial implementation for Instant Search using the Search and Query Loop blocks. Added as a new experiment on the Gutenberg Experiments page.

Related to #63053

How does it work?

  1. Insert a Query block anywhere on your site.
  2. Make sure to disable Force Page Reload i.e. use "enhanced pagination" in that Query block.
  3. Insert a Search block anywhere inside of that Query block.

When the Search block is inside of the Query block using "enhanced pagination", it automatically gets "updated" to an Instant Search block.

Any search input in the Instant Search block gets passed as the ?instant-search=<your-search> query search param in the URL.

Multiple Query & Search blocks

It's possible to have multiple Instant Search blocks in a page/post/template. In such a case, ensuring their functionality is isolated is essential. It's also important to remember that the "query type" in the Query block can be Default or Custom. The Default query is inherited from its respective template. For this reason, the Instant search block uses different URL search params when handling each:

  • Default - ?instant-search=<search-term>
  • Custom - ?instant-search-<queryId>=<search-term>

Limitations

⚠️ Multiple Instant Search blocks using the Default query on in the same template are currently not supported.
⚠️ The Instant Search block does not yet work correctly when placed in the Search template. See #63147 (review).

Pagination

The instant search functionality intersects with pagination of the Query block:

  1. Every time the search is updated, the pagination of its respective query is reset back to page 1.
  2. The pagination numbers and next/previous, are updated to reflect the number of results in the search.
  3. Clearing the search also resets the pagination back to page 1.

Further work

This is an initial prototype and intentionally does not yet implement all the features that should be expected in the final version. Those include but are not limited to:

  • Ensure that it works correctly for inherited queries.
  • Pagination - Reset page number for every search.
  • The search parameter in non-inherited queries should work correctly (as mentioned in https://github.com/WordPress/gutenberg/pull/63147#issuecomment-2214460127)`
  • Should work when there is more than one Query Loop block & Search block on the page.
  • Ensure all user input is sanitized / validated.
  • Correctly handle multiple Default queries.
  • a11y - announce search results. This needs testing but should work out of the box with the a11ySpeak() of the interactivity-router here and here.

Video

output_62afd8.mp4

@github-actions github-actions bot added the First-time Contributor Pull request opened by a first-time contributor to Gutenberg repository label Jul 4, 2024
Copy link

github-actions bot commented Jul 4, 2024

👋 Thanks for your first Pull Request and for helping build the future of Gutenberg and WordPress, @r-chrzan! 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.

@r-chrzan r-chrzan changed the title adding the necessary directives Instant search implementation Jul 4, 2024
@r-chrzan r-chrzan marked this pull request as ready for review July 5, 2024 20:54
@r-chrzan r-chrzan requested a review from ajitbohra as a code owner July 5, 2024 20:54
Copy link

github-actions bot commented Jul 5, 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: @rchrzan@whitelabelcoders.com, @ktmn.

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: rchrzan@whitelabelcoders.com, ktmn.

Co-authored-by: luisherranz <luisherranz@git.wordpress.org>
Co-authored-by: michalczaplinski <czapla@git.wordpress.org>
Co-authored-by: sirreal <jonsurrell@git.wordpress.org>
Co-authored-by: cbravobernal <cbravobernal@git.wordpress.org>
Co-authored-by: r-chrzan <rchrzan@git.wordpress.org>
Co-authored-by: gziolo <gziolo@git.wordpress.org>

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

@luisherranz luisherranz added [Block] Search Affects the Search Block - used to display a search field [Block] Query Loop Affects the Query Loop Block [Feature] Interactivity API API to add frontend interactivity to blocks. labels Jul 8, 2024
@luisherranz luisherranz linked an issue Jul 8, 2024 that may be closed by this pull request
@luisherranz luisherranz added the [Type] Feature New feature to highlight in changelogs. label Jul 8, 2024
@luisherranz luisherranz changed the title Instant search implementation Query and Search blocks: support for Instant Search Jul 8, 2024
@WordPress WordPress deleted a comment from github-actions bot Jul 8, 2024
@luisherranz
Copy link
Member

Great work!! 👏👏

Let's see what's next 🙂

  1. Modifying the query in inherited queries.

    I don't know if you've noticed, but the query_loop_block_query_vars filter only works when the "Inherit query from template" option is disabled.

    Screenshot 2024-07-08 at 16 33 20

    We should check if the query is inherited, and if so, apply the modification to the general query so it works in both cases.

  2. The search parameter in inherited queries.

    It's interesting that we can't use the search parameter ?s= because, in that case, the page that loads is the Search template.

    I'm going to think about it a bit, but it is true that it might not be possible to simply use ?s=. If we can't use it, then we need to think carefully about what the most suitable parameter is. Maybe ?is= for instant search?

  3. The search parameter in non-inherited queries.

    When the "Inherit query from template" option is disabled, the query pagination uses a custom search parameter instead of the regular ?paged=2//page/2 of the inherited queries: ?query-X-page=P where X is the query id ($block->context['queryId']).

    In that case, we should use a similar parameter: ?query-X-search=test.

  4. Pagination when searching.

    You commented in Slack:

    sometimes, when we are on e.g. page 3 and typing something, i see prev link button without results

    That's what happens when the new results don't have as many pages as the page you were on just before you started the search.

    I think we should try resetting the page number every time a new search is performed. In other words, if the user starts a search on page 3, we should remove the search param from the URL before navigating.

    • User is in /page/3 or ?query-X-page=3.
    • User types new characters.
    • We add the search parameter, but remove the pagination: /?is=test.

    This is probably going to present problems, as it might not be that easy to remove the pagination from the URL in the client for certain permalinks, but I think it's worth a try and we'll see what happens. If that's the case, one option to investigate would be to generate the URL of the first page on the server and send it to the client via the state.

  5. Search button closing itself during navigation.

    You commented in Slack:

    it doesn’t work very well when search is just a button. sometimes toggle closes while typing

    That's because the context has a property called isSearchInputVisible which is set to false on the server. So when a navigation occurs, it gets reset to false.

    The way to solve this is to transform that property into an initial property and have a client-only property so that the server property is only checked on the first page load and then only the client-only property is considered.

    I made a commit to implement this pattern by adding this type of getter:

    get isSearchInputVisible() {
      const ctx = getContext();
      if ( typeof ctx.isSearchInputVisible === 'undefined' ) {
        return ctx.isSearchInputInitiallyVisible;
      }
      return ctx.isSearchInputVisible;
    },

@michalczaplinski michalczaplinski changed the title Query and Search blocks: support for Instant Search - initial prototype Query and Search blocks: support for Instant Search Oct 31, 2024
Copy link
Member

@gziolo gziolo left a comment

Choose a reason for hiding this comment

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

I was testing the use cases outlined in my previous comment #63147 (comment).

I see that there are two issues still present. The proposed URL structure is incompatible with how WordPress works currently on the search results page. In particular, it is impossible to use two different URL params to control the same feature: s and instant-search. I tested it by modyfing the Search Results template and moving the Search block inside Query block.

Screenshot 2024-10-31 at 14 46 12 Screenshot 2024-10-31 at 14 46 38

Example URL:

https://playground.wordpress.net/scope:ambitious-noisy-town/?s=hello&instant-search=none

There is also a change in the behavior of the Search block with the button when the instant search is enabled. Without the feature, the user needs to click the button to trigger the search. With the feature, it gets triggered automatically on every keystroke. It's something to discuss further as it's a substantial change in the user experience.

lib/experimental/editor-settings.php Outdated Show resolved Hide resolved
@@ -187,6 +187,18 @@ function gutenberg_initialize_experiments_settings() {
)
);

add_settings_field(
'gutenberg-search-query-block',
__( 'Instant Search and Query Block', 'gutenberg' ),
Copy link
Member

Choose a reason for hiding this comment

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

What's the reasoning behind adding an experiment for this feature? In practice, it's just a new behavior on the Search block when placed inside 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.

I reasoned that the feature should remain an experiment while we polish it.

I don't want the current PR to stay up for weeks or months. Instead, I'd prefer to merge an experimental version of the block, even if it's incomplete, and add the remaining functionality in follow-up PRs.

packages/block-library/src/search/index.php Show resolved Hide resolved
@michalczaplinski
Copy link
Contributor

michalczaplinski commented Oct 31, 2024

Thanks for the review @gziolo!

You're right on both counts:

  1. Instant search does not yet work correctly in the Search template. I should have mentioned it in the PR description (updated now; see "Limitations"). If we can leave the block as experimental, I'd prefer to add it in the following PR to keep the current PR smaller.
  2. The search button is indeed somewhat redundant because the results update as you type. I'm not sure what the correct behavior should be. Let's discuss below.

The PR is ready for another round of review, note the updated PR description.

Here are some of the open questions that I'd like to discuss.

Search query URL param syntax

  1. Which syntax to use for the URL search param for the Default query? Should we use instant-search= (used currently in this PR) or is= or something else? I slightly prefer ?instant-search= because it's more explicit.

  2. Which syntax to use for the URL search param for the Custom query?

    • ?instant-search-123=test. This is the current syntax in this PR. It's consistent with the Default query syntax (?instant-search=).
    • ?query-123-search=test. This syntax was suggested here and is more consistent with query-123-page.
    • ?query-123-instant-search=test. More verbose but consistent with both query-123-page and Default query syntax (?instant-search=).
  3. Pagination for Default queries uses the /page/2/ format when using pretty permalinks. Should it be removed from the URL? Here we have two options:

    • Remove it from the URL. Downside is possibly breaking links as we have to parse the URL with a regex and remove the /page/X/ part.
    • Keep it in the URL and use the ?paged URL param. This works fine in my tests and redirects the request to the correct page. E.g. /page/2/?paged=3 will redirect to /page/3/. The downside is that it requires a redirect so is slightly slower.

Search submission behavior

  1. How should submitting the search query (using the Search button of the Search block or pressing Enter) work now?

    Currently, it takes you to the Search template for the given search query, but that's probably not what we want since the Instant Search already provides the result.

  2. Should the Search block inform the user in the editor that when it's placed inside of the Query block, it becomes an "instant search"?

Those two questions are related in my mind. If we can transparently inform the user in the editor that the Search block becomes an Instant Search block, then this Instant Search block should also have the correct behavior on the frontend that is different from the default Search block. The specific mechanism for doing this is not yet clear. Maybe we should be more explicit and add a toggle in the block settings that makes the Search block an Instant Search block?

Regardless of the solution in the editor, the Instant Search block should (on the frontend):

  1. Never show the Submit button. It's not necessary and redundant.
  2. Never submit the search query when pressing Enter.

Technical questions

  1. The current implementation runs a new Query for many of the blocks when Instant Search is enabled. It seems fine, but I want to ensure it's not causing performance issues. Examples: here, here, here or here.

  2. I also have some lingering doubts about temporarily substituting and then restoring the global $wp_query in query-pagination-next.php.

cc @DAreRodz @gziolo @sirreal @r-chrzan

Comment on lines +68 to +77
// Store the original global $wp_query
$temp_query = $wp_query;

// Temporarily replace global $wp_query with a new query that includes the
// search query because get_next_posts_link() uses global $wp_query.
$wp_query = new WP_Query( $args );
$content = get_next_posts_link( $label );

// Restore the original global $wp_query
$wp_query = $temp_query;
Copy link
Contributor

@michalczaplinski michalczaplinski Oct 31, 2024

Choose a reason for hiding this comment

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

Are there any issues with replacing the global $wp_query and restoring it after calling get_next_posts_link() ?

@gziolo
Copy link
Member

gziolo commented Nov 5, 2024

Limitations

⚠️ Multiple Instant Search blocks using the Default query on in the same template are currently not supported.
⚠️ The Instant Search block does not yet work correctly when placed in the Search template. See #63147 (review).

That’s fair. I’m fine with adding enhancement in steps. What’s the reasoning for adding the global handling for the default query in this PR if it isn’t expected to be functioning in the current shape?

The current implementation runs a new Query for many of the blocks when Instant Search is enabled. It seems fine, but I want to ensure it's not causing performance issues.

It’s definitely something that needs to be further investigated. What’s the reason the same query needs to be recomputed for every child block? Can it be shared? What prevents from using a WP filter to modify the vars passed to the existing query in Gutenberg?

https://github.com/WordPress/wordpress-develop/blob/25d71f18fa9784f8a6d0e94518e7f04e2c17ecac/src/wp-includes/blocks.php#L2617

Pagination for Default queries uses the /page/2/ format when using pretty permalinks

Is there a way to pass a canonical URL without the pagination applied to use it instead without all the complexity explained?

@sirreal
Copy link
Member

sirreal commented Nov 5, 2024

Pagination for Default queries uses the /page/2/ format when using pretty permalinks. Should it be removed from the URL? Here we have two options:

  • Remove it from the URL. Downside is possibly breaking links as we have to parse the URL with a regex and remove the /page/X/ part.
  • Keep it in the URL and use the ?paged URL param. This works fine in my tests and redirects the request to the correct page. E.g. /page/2/?paged=3 will redirect to /page/3/. The downside is that it requires a redirect so is slightly slower.

Is the information for how a URL should be constructed for a given site available somehow?

@michalczaplinski
Copy link
Contributor

That’s fair. I’m fine with adding enhancement in steps. What’s the reasoning for adding the global handling for the default query in this PR if it isn’t expected to be functioning in the current shape?

That's a good point. I'm happy to handle the default queries in another PR if that makes is easier to review. I will split the current PR up.

What’s the reason the same query needs to be recomputed for every child block? Can it be shared? What prevents from using a WP filter to modify the vars passed to the existing query in Gutenberg?

The reason is that the query_loop_block_query_vars filter does not run for the global queries (inherited from template). See this explanation.

In the follow-up PR (handling the global/inherited query) I can try to use the pre_get_posts filter instead. That should be the appropriate filter for filtering the global/inherited query.

Is there a way to pass a canonical URL without the pagination applied to use it instead without all the complexity explained?

I'm not aware of one. I took a look, and there is a wp_canonical_url filter, but that only works for posts and will not work for archives, for example.

In general, I don't think it is possible to do it reliably server-side in PHP. A site could also, e.g., sit behind an Nginx proxy, which could be serving on a different URL and a WordPress function would have no way of knowing it.

@michalczaplinski
Copy link
Contributor

Is the information for how a URL should be constructed for a given site available somehow?

@sirreal I think that the closest resource would be https://developer.wordpress.org/themes/basics/template-hierarchy/

The final URL depends on both the permalink settings and the Template hierarchy. Is that what you had in mind?

@sirreal
Copy link
Member

sirreal commented Nov 8, 2024

Sorry that was unclear, I was wondering if the site can be queried to determine whether a search param should be used or some other form.

For this initial version the redirect seems alright and it can be improved in the future. It may be able to observe the redirect and adjust behavior accordingly.

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 [Block] Search Affects the Search Block - used to display a search field [Feature] Interactivity API API to add frontend interactivity to blocks. First-time Contributor Pull request opened by a first-time contributor to Gutenberg repository [Type] Feature New feature to highlight in changelogs.
Projects
Status: 🏈 Punted to 6.8
Development

Successfully merging this pull request may close these issues.

Query and Search blocks: support for Instant Search
6 participants