-
Notifications
You must be signed in to change notification settings - Fork 16
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
2926: Fallback language in search function #3024
base: main
Are you sure you want to change the base?
Conversation
Does this issue already exist? Could you please link it here? Thanks :) |
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.
Tested on firefox and android.
One thing I noticed: If the native app is still loading possible search results, the search bar is not shown. Even though this was already the case before, this change makes it more visible since there are two hooks loaded now at the same time. I also experienced it to disappear during typing alleinerziehend
as probably the German results where not yet available. We can also create a separate issue for this though.
native/src/routes/SearchModal.tsx
Outdated
const mainResults = useSearch(allPossibleResults, query) | ||
const fallbackResults = useSearch(allPossibleFallbackResults, query) | ||
|
||
const searchResults = mainResults?.length === 0 ? fallbackResults : mainResults |
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.
🤔 Hmmm.... I think minisearch often returns search results even if there are no direct matches in the content. What is the reason to not always returning both (main results first, fallback concatenated afterwards)?
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.
Mainly that the issue description said to show the fallback ones if there aren't any in the selected language. Since Steffi opened the issue and she's been in charge of the target group, I assumed that that had been discussed.
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.
But yes, we currently have set minisearch to use a slightly fuzzy search, so it does often return results even without exact matches.
We could think about some kind of hierarchy that we would want results to show up in, e.g. 1. exact results in the selected language, 2. exact results in German, 3. fuzzy results in the selected language, 4. fuzzy results in German. Or maybe no fuzzy results in German? 🤔
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.
Hmm, perhaps e.g. @hauf-toni has some opinion on this. Personally I think it would definitely not hurt to always append German search results. That way, the focus is still on results in the users language but it is also easy to get German results if needed without needing any complicated hierarchy or similar. Just my personal thoughts though, curious if Toni has another opinion on this.
const useMemoizeResults = ( | ||
categories?: CategoriesMapModel | null, | ||
events?: EventModel[] | null, | ||
pois?: PoiModel[] | null, | ||
) => useMemo(() => formatPossibleSearchResults(categories, events, pois), [categories, events, pois]) |
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.
💭 Do we really have to memoize this? Doesn't really look calculation intense 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.
Good point, that is probably not necessary 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.
Turns out we do need to memoize this because of JS checking for equality for arrays (/ objects) only by reference and not by content. Without memoization, the page keeps refreshing because React thinks we have a new list of results.
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.
But miraculously, this is only an issue in web, not in native. So we need the memoization in web to avoid an infinite re-rendering loop, but not in native. I am confused.
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 can have a look some time the next few days, perhaps I have an idea why this leads to rerendering otherwise
web/src/routes/SearchPage.tsx
Outdated
}) | ||
const fallbackResults = useSearch(allPossibleFallbackResults, query) | ||
|
||
const results = mainResults?.length === 0 ? fallbackResults : mainResults |
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.
🤔 See native above
const allPossibleResults = useMemo( | ||
() => [...(categories?.toArray().filter(category => !category.isRoot()) || []), ...(events || []), ...(pois || [])], | ||
[categories, events, pois], | ||
() => formatPossibleSearchResults(categories.data, events.data, pois.data), | ||
[categories.data, events.data, pois.data], | ||
) |
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.
🔧 Same as for native
const { data: categories, ...categoriesReturn } = useLoadFromEndpoint(createCategoriesEndpoint, cmsApiBaseUrl, params) | ||
const { data: events, ...eventsReturn } = useLoadFromEndpoint(createEventsEndpoint, cmsApiBaseUrl, params) | ||
const { data: pois, ...poisReturn } = useLoadFromEndpoint(createPOIsEndpoint, cmsApiBaseUrl, params) | ||
const categories = useLoadFromEndpoint(createCategoriesEndpoint, cmsApiBaseUrl, params) |
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.
💭 In theory we could perhaps add a parameter include root
set to true as default or something to the categories endpoint so we don't have to filter it out after again?
Probably only really works well for web though, so not sure if its a good idea.
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.
Good point, why are we filtering out isRoot
anyway?
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.
Because the root is manually added by us to allow for our tree-based navigation structure. However, we don't want to display an empty category without title/icon/content in the search. Does that make sense?
Sorry, I didn't mean that it will be a separate issue but a separate PR. I'll continue with that next. |
I fixed the first part of the issue, the search bar is now shown during loading. This should also have fixed the second part, the short disappearance while typing a German word, but can you check again? |
b836450
to
abe32c0
Compare
Short description
When your app is set to e.g. English and you look for a German term that you got a letter about like "Bürgergeld", you get zero results because we only search for English results. This PR adds a fallback language (the one that we translate from, i.e. German), so in this case, if there are no results in English, we then look in German.
The feedback screens have not been updated yet, that will be a separate
issuePR.Proposed changes
Side effects
Testing
You can switch the app to English and search for "alleinerziehend".
Resolved issues
Partially fixes: #2926