Skip to content
This repository was archived by the owner on Sep 30, 2024. It is now read-only.

search: fix regexp toggle if regexp is default#63489

Merged
stefanhengl merged 3 commits into
mainfrom
sh/toggle
Jun 27, 2024
Merged

search: fix regexp toggle if regexp is default#63489
stefanhengl merged 3 commits into
mainfrom
sh/toggle

Conversation

@stefanhengl

@stefanhengl stefanhengl commented Jun 26, 2024

Copy link
Copy Markdown
Member

I noticed that the regexp toggle doesn't work anymore if "search.defaultPatternType": "regexp". This is related to a recent change #63410.

We also append patternType:keyword in that case which I don't think we want, because we have an UI element to indicate that keyword search is active.

The question I don't know how to answer is: What should happen if regexp is the default and the user toggles keyword search off. Should we go back to regexp or to standard?

To reproduce the original problem, just configure regexp as default and toggle regexp on/off in the search bar.

Test plan:

  • new unit test
  • manual testing with default pattern type set to "keyword" and "regexp".

I noticed that the regexp toggle doesn't work anymore if the
`"search.defaultPatternType": "regexp"`.

We also append `patternType:keyword` in that case which I don't think we
want, because we have an UI element to indicate that keyword search is
active.

The question I don't know how to answer is: What should happen if
regexp is the default and the user toggles keyword search off. Should we
go back to "regexp" or to "standard"?

Test plan:
manual testing with default pattern type set to "keyword" and "regexp".
@stefanhengl stefanhengl requested a review from jtibshirani June 26, 2024 14:16
@cla-bot cla-bot Bot added the cla-signed label Jun 26, 2024
@github-actions github-actions Bot added team/product-platform team/search-platform Issues owned by the search platform team labels Jun 26, 2024
@stefanhengl stefanhengl marked this pull request as ready for review June 26, 2024 14:16

@jtibshirani jtibshirani left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thank you for fixing this! I should've tested it. It'd be great to add a test case if it's not too hard (I am not 100% how to do this myself, as it requires triggering the toggle 🤔 )

Comment thread client/branded/src/search-ui/input/toggles/Toggles.tsx
@stefanhengl stefanhengl merged commit cb41db3 into main Jun 27, 2024
@stefanhengl stefanhengl deleted the sh/toggle branch June 27, 2024 10:50
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

cla-signed team/product-platform team/search-platform Issues owned by the search platform team

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants