Fix old results shown on predictive search

[FCalabria]

PR Summary:

Hide stale results on the predictive search when writting a new query and refresh the "search for" button with the new term as soon as possible.

Why are these changes introduced?

Fixes #2199 and improves the user experience updating the "search for" button while the user types, instead of waiting for the predictive search query to resolve.

What approach did you take?

To fix the bug, delete the results and show the loading spinner when the new query is not an addition over the old one.

For the improvement, I used a search and replace technique modifying the html on the fly with JS. I could not figure out a way to identify the search term that wasn't just looking for the string itself (depending on the language the quote symbol and the term position changes). This can cause a problem when the user looks for exactly the label on the search button ("Search for...") which i avoided not replacing anything if there is more than one match.

Testing steps/scenarios

• When writing a query, the search term in the button updates before the predictive search results
• When a query is total or partially deleted and then a different term is written, the old predictive search results are not shown, just a loading state.

Demo links

• Store
• Editor

Checklist

• Added PR summary for release notes
• Requested review from UX (Only for changes that are affecting the experience or perceivable visual details)
• Created a ticket for the help.shopify.com documentation team about updates to theme settings. (Internal-only task)
• Followed theme code principles
• Linted with Theme Check
• Tested on mobile
• Tested on multiple browsers
• Tested for accessibility

[NathanPJF]

Separating from the button SVG can removed here by wrapping the translation string in a span and searching for that; I'd recommend a data-attribute of data-search-term over a class or any other selector type.

<span data-search-term>{{ 'templates.search.search_for' | t: terms: predictive_search.terms }}</span>

Note: There is a way in Liquid to put HTML in the translation string so you could wrap just the query, but I think that makes the translation string too difficult to work with.

[ludoboludo]

I think I'd suggest more grabbing the icon via it's class or element name 🤔 searchForButton.querySelector('svg')
It would be easier to read.

[NathanPJF]

My point is that I don't believe this function even needs to know that there is an svg in the HTML. Right now, this function is taking the innerHTML of a <button> and changing it by concatentating a string and an svg. However, there's nothing about the svg that is going to change, so why include it? Adding a new span would give the function a target element to inspect the text on.

  updateSearchForTerm(previousTerm, newTerm) {
    const searchForText = this.querySelector("#search-for-button [data-search-term]");
    const currentText = searchForButton?.innerText;
    if (currentText) {
      if (currentText.match(new RegExp(previousTerm, "g")).length > 1) {
        // The new term matches part of the button text and not just the search term, do not replace to avoid mistakes
        return;
      }
      searchForText.innerText = currentButtonText.replace(previousTerm, newTerm);
    }
  }

[NathanPJF]

Can you use the existing closeResults method for this instead? Is this additional method necessary?

[ludoboludo]

Looking good. Ill test a bit tomorrow some scenarios.
But no bugs/issue noticed 👍

[ludoboludo]

I can see why you'd have the __ underscores there. But since we didn't use that same pattern for predictive-search-option-search-keywords I think we should approach it the same way. With a - instead.

[ludoboludo]

This doesn't do what the function you had before did 🤔
Here are two video comparisons: using clearResources() & using closeResults(false)

[FCalabria]

I tested and it seemed to be fine, but maybe my internet was too fast to appreciate the difference. Thanks for the thorough testing!

[ludoboludo]

I think it's looking good 👍
I can see how the proposed change by Nathan would make sense too 👍 About wrapping the text in a span.
Not sure if we need the data attribute since we can select the span element itself.

[NathanPJF]

Thanks for owning this @FCalabria

It doesn't seem like the failing theme check step will be a blocker to merge. I see that the recent PRs have merged without this passing (example). But don't forget to rebase this first 🔂 🚢

[FCalabria]

I think it's looking good 👍 I can see how the proposed change by Nathan would make sense too 👍 About wrapping the text in a span. Not sure if we need the data attribute since we can select the span element itself.

I decided to use the data attribute because with the span i don't need the reference to the full button anymore, so i could delete that id.

It doesn't seem like the failing theme check step will be a blocker to merge. I see that the recent PRs have merged without this passing (#2253 (comment)). But don't forget to rebase this first 🔂 🚢

Sure! I was waiting for you to finish reviewing to not mess up anything

[NathanPJF]

🎩 I just re-tophatted https://os2-demo.myshopify.com/?preview_theme_id=139285364758

[ludoboludo]

🎉