This repository was archived by the owner on Sep 30, 2024. It is now read-only.
search: activate smart search by default#44395
Merged
Merged
Conversation
55d0d2a to
11c1096
Compare
Contributor
|
Codenotify: Notifying subscribers in CODENOTIFY files for diff 923a15a...562ddb1.
|
3a208df to
f1c3143
Compare
Bundle size report 📦
Look at the Statoscope report for a full comparison between the commits 562ddb1 and ab43aac or learn more. Open explanation
|
rvantonder
commented
Nov 15, 2022
Comment on lines
80
to
92
Contributor
Author
There was a problem hiding this comment.
We don't decide anything based on defaultPatternType any longer.
rvantonder
commented
Nov 15, 2022
Comment on lines
194
to
199
Contributor
Author
There was a problem hiding this comment.
smart toggle is always shown. its default state can be set by search.defaultMode.
rvantonder
commented
Nov 15, 2022
Contributor
Author
There was a problem hiding this comment.
On app load default mode is SmartSearch (can be overridden by search.defaultMode setting).
novoselrok
approved these changes
Nov 15, 2022
novoselrok
left a comment
Contributor
There was a problem hiding this comment.
LGTM. Tried it out locally, default mode flag works.
f1a5a40 to
6857c6b
Compare
6857c6b to
6eacae1
Compare
636d04a to
0da9af8
Compare
0da9af8 to
562ddb1
Compare
rvantonder
added a commit
that referenced
this pull request
Nov 16, 2022
This reverts commit 3bc1d5c.
This was referenced Nov 16, 2022
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This activates the smart search feature by default (the toggle always shows up). It also adds a setting where the default toggle state can be set by
search.defaultMode.See inline comments for more details.
After this PR I will update changelog and continue testing/refining things.
Extra notes, about VSCode extension:
After some manual testing, I think it makes sense to keep the smart search toggle in VScode, which is what this PR does. Because:
It works as expected and consistent with web app behavior (this because the streaming search endpoint sees the smart search value)
It is really ugly and messy to try and introduce a setting/bool to not display certain toggle buttons when in VSCode mode and not worth the tradeoff IMO. We would effectively have a need for divergent UIs. If we really want to disable smart search for VSCode we should first do the work that completely separates it, and it's UI (
Toggles,SearchBox) from the web app. That does not seem like the desired approach, we want to keep webapp and VSCode components consistent.There are two cons to the tradeoff. The main one is that the VSCode extension doesn't display the alerts/info box (it doesn't appear to display any server side alerts). I think this is a gap in the extension that we should address swiftly. I wasn't aware of it being absent before, so this is already a divergence that really seems like something we should support in VSCode. Second, there isn't a direct URL that corresponds to the web app (at least, I'm not sure there is a web URL can copy from the VSCode environment). But at least, now if a user had smart search toggled in VSCode, and sees results, they can get the same behavior in the webapp as well. That can also be mitigated with a "open in browser" if that doesn't exist already.
Test plan
Existing tests + manual testing
App preview:
Check out the client app preview documentation to learn more.