-
-
Notifications
You must be signed in to change notification settings - Fork 950
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
Feature: search suggestions for search and quick launch #2775
Feature: search suggestions for search and quick launch #2775
Conversation
If this feture and the search feature of the `quicklaunch` are turned on, up to *four* search suggestions will be shown as results.
This commit removes the async wrapper in the useEffect again. Instead the search suggestions are stored in their own state and the `quicklaunch` results show the last suggestions until they are updated. This allows the search for services be fast even with a slow internet connection.
Now the search request is only sent, if the *searchString* changed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. I can take a look at the PR later but this should be opt-in not opt-out, otherwise we’re going to get a lot of complaints about this showing up out of nowhere.
in that case presumably the variable should be renamed to show…
Also FYI our guidelines now require 10 up-votes for new features but I realize the docs don’t show that for the latest version yet
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's also a bit odd that this works only for quick launch but comes from search. I think it should be implemented for both so they mirror each other. We'll need to make sure the UI for the search one is good.
This makes the search suggestions opt-in.
`.splice(0, 4)` returns the first 4 entries. Therefore we need to reasign `newSearchSuggestions[1]`
Regrading the search in the search bar: I made a quick implementation using the Furthermore, I would suggest moving the settings for the search provider, including the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for changes.
Yea the search widget auto-complete is a good start. As far as the design, it'd be easier if you push those changes and I can play with it, if youre OK with that.
And yea, maybe it seems overkill but users generally want more control especially as this is sending data (and getting tracked, of course) so I would include a setting for the widget (showSuggestions) in addition to the quick launch one. And I think something even better would be what we've done similarly elsewhere which is to let the search widget one act as the 'default' for quick launch (in other words if you set search widget showSuggestions
to true, the quick launch will do the same unless you explicitly set it to false). If you only want them in quick launch set it just in quick launch. That feels 'clean' and means a lot of users will only need to set it in one place (if you want suggestions in search widget theres a good chance you do in QL).
This can be activated by adding `showSearchSuggestions: true` to the search widget config.
I pushed the draft for the suggestions in the search bar. |
5f0924d
to
6db28b4
Compare
6db28b4
to
a442c4d
Compare
I think we are good here. |
This makes the Google search suggestion API return charset=utf-8.
Head branch was pushed to by a user without write access
I couldn't live with it. Looks like Google only returns UTF-8 if the User Agent includes |
Head branch was pushed to by a user without write access
I found another bug, I didn't URI encode the query again in the API route. This made it so that special characters were not correctly sent to the search suggestions API. But apart from that, I think we are fine. The thing with the Umlaut for Google doesn't bother me too much (I use DuckDuckGo anyway ;) ) While testing the Umlaut thing, I found another bug, but I will open an issue for that one. Edit: Okay, I can't open an issue. How do I report a Bug? Or should I just open an PR? |
You can open a discussion about it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work
…2775) --------- Co-authored-by: shamoon <4887959+shamoon@users.noreply.github.com>
…2775) --------- Co-authored-by: shamoon <4887959+shamoon@users.noreply.github.com>
…2775) --------- Co-authored-by: shamoon <4887959+shamoon@users.noreply.github.com>
…2775) --------- Co-authored-by: shamoon <4887959+shamoon@users.noreply.github.com>
…2775) --------- Co-authored-by: shamoon <4887959+shamoon@users.noreply.github.com>
This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new discussion for related concerns. See our contributing guidelines for more details. |
Proposed change
This PR adds search suggestions to the quick launch menu.
Currently, only the top four suggestions are shown, but it could be made adjustable in the config. I just wasn't sure where to put the setting. To the quick launch setting or the search widget?
Screenshot
Config
I added an option to the
quicklaunch
setting calledhideSearchSuggestions
which hides the suggestions.Furthermore, I added the option
suggestionUrl
to the custom search widget.This allows the user to set a custom URL for suggestions, like this:
The body of the response from the API configured with
suggestionUrl
should have the following format:The above example is also included in the change to the docs.
I found suggestion URLs for all implemented search providers and Ecosia which I used as an example for the custom provider.
Relates to #1225 and #1517
Type of change
Checklist: