Search: surface pattern type in query input#63326
Conversation
| patternTypeInput?: SearchPatternType | ||
| } | ||
|
|
||
| export function literalSearchCompatibility({ queryInput, patternTypeInput }: QueryCompatibility): QueryCompatibility { |
There was a problem hiding this comment.
Previously, we attempted to automatically convert patterntype:literal searches to patterntype:standard. It found it surprising to rewrite the user's query from under them after they submit it. Also, I don't see a strong reason to do this, since we still support the literal pattern type in the backend (and kind of need to, until we explicitly mark it unsupported).
This logic led to extra weird UX with this PR, where if your query has patterntype:literal, then when you submit it, we immediately replace it with patterntype:standard. So I just removed this.
There was a problem hiding this comment.
we still support the literal pattern type in the backend
Do we want to always support patterntype:literal though? I think this was intended to help nudge users in the direction of the newer search syntax. I expect we will still want ways to provide that nudge, especially if we ever want to try to get rid of the regex toggle. I don't feel strongly about this specific way of doing it though, so deleting this is fine with me.
There was a problem hiding this comment.
I think this was a reasonable approach at the time to teach people about the new standard syntax. But now literal usage is so rare that it's not worth keeping this around.
As a general note, I'd prefer if we don't rewrite people's queries in the UX without an explicit action. Swapping out the query is tricky, and discourages people from fixing the root cause of the old pattern type (perhaps it's a custom search engine or old link). In this PR we're gently encouraging them to switch to keyword, by making the old pattern type look obviously non-default and a bit clunky :)
|
Noting one slightly weird thing with this PR: the I don't think this is a big deal, especially since the vast majority of |
Could we avoid this by checking whether the query string already contains a EDIT: Ignore that. I see that we remove the pattern type from the query when we construct the URL, so the information about the position is lost. |
stefanhengl
left a comment
There was a problem hiding this comment.
There is another weird thing. We change from camelCase to lowercase for the same reason we don't have the positional information anymore.
I think the weirdness is acceptable since we don't expect users to actually type these pattern types very often.
|
I just found a bad bug: if you are in |
camdencheek
left a comment
There was a problem hiding this comment.
LGTM pending the regex toggle fix!
| patternTypeInput?: SearchPatternType | ||
| } | ||
|
|
||
| export function literalSearchCompatibility({ queryInput, patternTypeInput }: QueryCompatibility): QueryCompatibility { |
There was a problem hiding this comment.
we still support the literal pattern type in the backend
Do we want to always support patterntype:literal though? I think this was intended to help nudge users in the direction of the newer search syntax. I expect we will still want ways to provide that nudge, especially if we ever want to try to get rid of the regex toggle. I don't feel strongly about this specific way of doing it though, so deleting this is fine with me.
|
I fixed the toggle issue in 7a4af03. It turned out pretty tricky and not up to my usual standards :( I tried a few other things but couldn't find an alternative approach. I'm okay with it since we're rewriting all of this into sveltekit (and the architecture is looking much simpler...) |
Previously, enabling then disabling the regexp toggle would always set the patterntype to `keyword`, even when the user has set `search.defaultPatternType: standard`. Now, toggles always revert back to the default pattern type. To support this, this PR adds `defaultPatternType` to the nav bar query state, which is updated every time there's a settings update. Having `defaultPatternType` available also lets us fix another bug: when the default pattern type has been set to `standard` we no longer awkwardly show `patterntype: standard` in the search bar. (This confusing behavior was introduced recently in #63326.) **Overall, this PR set us up to remove `experimentalFeatures.keywordSearch`, along with the keyword search toggle.** To opt out of keyword search, users can just set `search.defaultPatternType`, and have it work everywhere.
We plan to remove the 'Keyword Search' toggle as part of bringing the feature
to GA. Once the toggle is removed, the search UX will only represent
keyword(the default pattern type) and
regexp(through the regex toggle), with novisual indication for other pattern types. So if a user clicks on a link using
patterntype:standard, the search will just behave differently, without anyindication in the UX as to why.
This PR surfaces the
patterntypefilter in the search bar whenever it's notkeywordorregexp. That way, users can see an old pattern type is beingused and understand why the search behavior may be different.
Relates to SPLF-68
Test plan
Added unit tests, manually tested search bar behavior with different pattern
types.