Skip to content
This repository has been archived by the owner on Jan 10, 2024. It is now read-only.

Centralized search v2 #1457

Merged
merged 11 commits into from
Jul 13, 2022
Merged

Centralized search v2 #1457

merged 11 commits into from
Jul 13, 2022

Conversation

PiotrKedra
Copy link
Contributor

@PiotrKedra PiotrKedra commented Jun 7, 2022

Issue

Fixes: #678

Description

More or less I want to create a centralized search, that was started in #1343 PR. Why am I doing it from scratch instead of using a branch from existing PR? Since this code was created 3 years ago I had a hard time merging it with the current master; there were too many conflicts. Therefore, I decided to start over again.

Additional to what was proposed in #1343 PR I am thinking of adding the history of searches, and maybe some tags (lecture, person, room, ...) for quicker navigation in search results.

@PiotrKedra
Copy link
Contributor Author

Search should be done. Here it the final look of it:

pic 1 pic 1
pic 1 pic 1
pic 1 pic 1

Discussion

If anyone has other suggestions on what more can be done here, I am open to adding them.

  • I was thinking of adding a "My lectures" type next to the "Lecture" type, but I am unsure if this is necessary.
  • Also, now search starts when the user clicks 'enter' (this is how it worked in previous searches). I was thinking that maybe it would be better if the search starts when the user is still typing the query, but then there will be lots of API calls to TUMOnline. My question is: is this a problem?

@PiotrKedra PiotrKedra marked this pull request as ready for review June 20, 2022 12:23
@CommanderStorm
Copy link
Member

CommanderStorm commented Jun 29, 2022

I tested your code and everything works as expected.
Looks awesome.

Just one small issue I noticed:
From a development perspective it would be nice, if the enter key does also run the search, not only the search-icon on the virtual keyboard.

From a performance perspective for Seach-as-you-type: @joschahenningsen what can our current backend handle?
Room-search should be no real problem for this one, since searching is infrequent and the current comparison is just a query to the database.
Though, I have no idea how this looks on the Persons/Lectures side.

@joschahenningsen
Copy link
Member

I have no worries, backend wise here :)

@joschahenningsen joschahenningsen self-requested a review June 29, 2022 05:50
@CommanderStorm
Copy link
Member

I have no worries, backend wise here :)

Just to make shure, that there has not been a miscomunication.
I noticed, that my statement may have been misleading:
Seach-as-you-type (as referenced in the discussion part) is okay with you?

@joschahenningsen
Copy link
Member

Seach-as-you-type (as referenced in the discussion part) is okay with you?

Right that should be no issue

@PiotrKedra
Copy link
Contributor Author

I have added search-as-you-type. The enter key still does not run the search, but now with search-as-you-type, I think it is not needed.

@kordianbruck kordianbruck merged commit 2c10c84 into TUM-Dev:master Jul 13, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Migrate Searches to unified SearchActivity
4 participants