Skip to content
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

API highlight control #179

Merged
merged 4 commits into from
Aug 6, 2022
Merged

API highlight control #179

merged 4 commits into from
Aug 6, 2022

Conversation

CommanderStorm
Copy link
Member

@CommanderStorm CommanderStorm commented Aug 1, 2022

Added the ability to control the highlighting behaviour.
As noticed in #103 It would be nice to make this customizable and not result in hacks as seen in TUM-Dev/Campus-Android#1462 (comment)
Resolves #114

Note:
I have changed the highlighting fields to <em>. I can revert this, if you'd like, but I assume this removes at least a bit of code in the webclient, so I thought this would be a better solution.

@CommanderStorm CommanderStorm self-assigned this Aug 1, 2022
@CommanderStorm CommanderStorm added server Related to the backend/server frontend Related to the frontend search Related to search UX labels Aug 1, 2022
@CommanderStorm CommanderStorm marked this pull request as ready for review August 1, 2022 23:39
Copy link
Contributor

@octycs octycs left a comment

Choose a reason for hiding this comment

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

Codewise I think this is fine (only the HTML escaping would be good to still have). The only reason I am not sure whether to switch to MeiliSearches highlighting now, is, that they still seem to treat 'typos' in numbers. For example:
image

It happens only for IDs longer than 4 characters, which are mainly only in the MW and CH buildings. It's not too bad, but I don't really like it. What do you think?

Edit: in some cases also 4 chars only:
image

webclient/src/modules/autocomplete.js Show resolved Hide resolved
@CommanderStorm
Copy link
Member Author

I am not sure whether to switch to MeiliSearches highlighting now, is, that they still seem to treat 'typos' in numbers. It's not too bad, but I don't really like it. What do you think?

Well…
But that is how the search results get produced.
The highlighting only shows where things match…

I do think that it is better to show that MS does typo-tolerance even in these circumstances…
We can potentially tweak the typo tolerance a bit (rel #115), but there is just not much we can do…

I honestly don't see why you think this is a problem… Could you explain your reasoning a bit more? 😅

@octycs
Copy link
Contributor

octycs commented Aug 2, 2022

I honestly don't see why you think this is a problem… Could you explain your reasoning a bit more? sweat_smile

When you go through the search results (in a quick manner), I think you mainly want to see as directly as possible whether this is the result you were looking for or not. And the highlighting helps you by showing you that the highlighted parts match exactly what you typed in.

I don't think that the typo-tolerance in search is bad, but I don't like it in the highlighting. I can understand that MeiliSearch does this, because it could be helpful if you search large text databases, but my problem is that it also applies to numbers.

I think our ID search is in general quite okay, but not ideal. So I don't object to merging this, it's just that I wouldn't want to leave it this way in the long run (It does also have advantages of course, especially Meilisearch doesn't highlight too much if your search contains a single digit).

@octycs octycs merged commit 89e8405 into main Aug 6, 2022
@octycs octycs deleted the highlighting branch August 6, 2022 11:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
frontend Related to the frontend search Related to search UX server Related to the backend/server
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Feature] Investigate better highlighting
2 participants