-
-
Notifications
You must be signed in to change notification settings - Fork 394
feat: add autocomplete for search engine #586
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
Merged
Merged
Changes from all commits
Commits
Show all changes
4 commits
Select commit
Hold shift + click to select a range
4fb78e1
feat: add autocomplete for mediatype, filetype, path, tag, and tag_id…
python357-1 92c71d2
fix: address issues brought up in review
python357-1 c77107c
fix: fix mypy issue
python357-1 e5e532c
fix: fix mypy issues for real this time
python357-1 File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
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
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
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.
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.
I think using the tag's "display name" here might be better, as that would reduce confusion between identical tag names. It would also be nice if the tag name text was colored in the dropdown, but speaking from personal experience I don't know of a way to do that without creating a completely custom widget from scratch, so I don't expect that from you here.
For the future, I'm picturing the search bar as something a bit more abstracted from the internal search query. For tags, this could mean that the user would see a tag label widget (alongside other query text) in the search bar while internally the query can refer to it by the tag's ID, eliminating any confusion between tags with the same name.

Example:
This is a bigger undertaking than the scope of this PR though, and having the functionality here is already a great help over having no auto-completion at all.
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.
potentially dumb question: what is a tag's display name? its' shorthand?
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.
I've just realized that display names didn't make the jump from JSON... If #534 doesn't implement them then they'll need to be implemented in another new PR.
Essentially, they're a version of a Tag's named combined with their first parent tag in parenthesis (with preference to that parent tag's shorthand) to aide in telling tags with the same name apart. In 9.4 they were generated and grabbed from a method rather than being stored directly, which likely contributed to them going unnoticed during the migration so far.
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.
Tomorrow I'll look into getting the display names add in on #534, no point in having another PR just for that.