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

Search UI #627

Merged
merged 15 commits into from
Dec 11, 2024
Merged

Search UI #627

merged 15 commits into from
Dec 11, 2024

Conversation

alfredgrip
Copy link
Contributor

@alfredgrip alfredgrip commented Dec 6, 2024

This PR rewrites search in frontend so that it uses the Meilisearch endpoint introduced in #611

It also adds a "advanced search" that allows filtering search on different indexes

It also fixes some minor TS misses that slipped through in the #611 PR

To try out this PR, please ensure that Meilisearch is up and running on your Docker (rerun the dev/setup_db.sh script) and then head to localhost:.../admin/debug and press Sync with Meilisearch

@alfredgrip alfredgrip linked an issue Dec 6, 2024 that may be closed by this pull request
Copy link
Member

@Isak-Kallini Isak-Kallini left a comment

Choose a reason for hiding this comment

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

Search in english doesn't seem to be working, for example:
The bottom article comes up when searching for test
image
But when searching for its title it doesn't show up:
image
This is with language set to english, the article also comes up if i search for it's swedish title

Also if I press backspace while the input is empty the spinner appears until I type something

But looks great overall, feels very nice to use. Big improvement compared to before!

src/routes/(app)/search/+page.server.ts Outdated Show resolved Hide resolved
src/routes/(app)/search/+page.server.ts Outdated Show resolved Hide resolved
src/lib/components/search/ArticleSearchResult.svelte Outdated Show resolved Hide resolved
src/lib/components/search/GlobalSearch.svelte Outdated Show resolved Hide resolved
@alfredgrip
Copy link
Contributor Author

alfredgrip commented Dec 7, 2024

Search in english doesn't seem to be working

I found the issue for this. The /search page does a fetch to the api endpoint (which in turn makes a request to Meilisearch). When the server on the search page gets a request, the event.locals.language gets populated to the correct value by hooks.server.ts. But when the /search does a fetch to the API endpoint, the server there checks for event.locals.language but it defaults to swedish.
Tried some console logging and found this when trying to search with language set to en:

search param language is: en      // query parameter
search locals language is: en     // event.locals.language
API param language is: en         // query parameter
API locals language is: sv        // event.locals.language

So either the hooks.server.ts is faulty, or the /search server needs to do the fetch some other way. When the API page instead gets language from the query parameter, search in english works as intended.

@danieladugyan Do you have any thoughts about this?

@alfredgrip
Copy link
Contributor Author

alfredgrip commented Dec 8, 2024

I found the issue for this. The /search page does a fetch to the api endpoint (which in turn makes a request to Meilisearch). When the server on the search page gets a request, the event.locals.language gets populated to the correct value by hooks.server.ts. But when the /search does a fetch to the API endpoint, the server there checks for event.locals.language but it defaults to swedish. Tried some console logging and found this when trying to search with language set to en:

search param language is: en      // query parameter
search locals language is: en     // event.locals.language
API param language is: en         // query parameter
API locals language is: sv        // event.locals.language

So either the hooks.server.ts is faulty, or the /search server needs to do the fetch some other way. When the API page instead gets language from the query parameter, search in english works as intended.

I found that this can simply be fixed if the /search server does a request to /en/api/search if locals.language == "en". I'm not a big fan of this, however. Having two "different" API endpoints that does the same, but which could easily be merged in to a single one by having a query parameter i.e. ?language="en", seems weird. I think it makes more sense to have a single one that accepts the language query parameter. What are your thoughts @Isak-Kallini @danieladugyan ?

E.g. I don't like the look of this:

let url;
if (event.locals.language === "sv") {
  url = new URL("/api/search", event.request.url);
} else {
  url = new URL("/en/api/search", event.request.url);
}

@danieladugyan
Copy link
Contributor

I found that this can simply be fixed if the /search server does a request to /en/api/search if locals.language == "en". I'm not a big fan of this, however. Having two "different" API endpoints that does the same, but which could easily be merged in to a single one by having a query parameter i.e. ?language="en", seems weird. I think it makes more sense to have a single one that accepts the language query parameter. What are your thoughts @Isak-Kallini @danieladugyan ?

Sorry, that's my bad for introducing the bug. Anyways, that's exactly how I think it should work. It's consistent with how language is handled everywhere else in the app.

  1. The request hits the server.
  2. paraglide.js looks at the URL and check if it has a language prefix, i.e /en, and sets the language in event.locals accordingly.
  3. Route handlers use event.locals.language as needed - the Prisma translationExtension doing this automatically is enough in most cases.

In summary, if we're on the Swedish site, fetch normally, otherwise fetch with 'en' prepended. Ideally this would be done automatically by the library, but maybe we could make a fetch wrapper?

@alfredgrip
Copy link
Contributor Author

I think it's weird though, specifics sent to an API endpoint should IMO be in the query parameters, not extracted from the URL. But I guess this works.

Just pushed some changes that fixes the english search and also handle translation mapping.

but maybe we could make a fetch wrapper?

So we would write a wrapper for the already existing event.fetch wrapper from SvelteKit? 🫠

@Isak-Kallini
Copy link
Member

Isak-Kallini commented Dec 8, 2024

I think that solution looks fine since, as Daniel says, that's how language is handled everywhere else.
But should maybe use i18n.resolveRoute('/api/search')?

@alfredgrip
Copy link
Contributor Author

I think that solution looks fine since, as Daniel says, that's how language is handled everywhere else. But should maybe use i18n.resolveRoute('/api/search')?

Didn't know that was a thing, but that's a nice way to handle it I guess!

Copy link
Member

@Isak-Kallini Isak-Kallini left a comment

Choose a reason for hiding this comment

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

One small comments, looks good otherwise.
Great job!

{/if}
{#if form?.message}
<div class="alert alert-error mt-4">
Error: {JSON.stringify(form)}
Copy link
Member

Choose a reason for hiding this comment

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

form.message looks nicer?

@alfredgrip alfredgrip merged commit d6dce6a into main Dec 11, 2024
3 checks passed
@alfredgrip alfredgrip deleted the search-ui branch December 11, 2024 15:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Missing functionality when searching
3 participants