feat: Search filtering logic

[alextran1502]

[cloudflare-workers-and-pages]

Deploying with    Cloudflare Pages

Latest commit:	4cfc5b0
Status:	⚡️  Build in progress...

View logs

[mertalev]

You can move the filter to the query. e.country should also never be nullish.

[danieldietzler]

I was thinking that we always return an object of all search options. This way, we could update the suggestions on select of an option, not when opening a combobox. Should help with performance a lot.

[alextran1502]

I was thinking that we always return an object of all search options. This way, we could update the suggestions on select of an option, not when opening a combobox. Should help with performance a lot.

I was debating on this, it means we will do the filtering on the client. I opt-in for filtering on the server instead, to keep the payload light.

[mertalev]

Some suggested cleanup for the web code and a general design question for how we want to handle the search DTO

[mertalev]

This dto is kinda awkward. It isn't clear when these fields will get used. For example, if the type is STATE, then only country is used: state, make and model are ignored. But you can't tell that from looking at this.

I don't know if we ever settled on whether we want to do the filters in separate calls or in the same one, but this dto only makes sense if we intend to move toward getting all the suggestions in the same call down the line.

[mertalev]

Similar to the above comment, having a single method for this only makes sense if we want to change this later to return all suggestions. If we want to continue to have separate calls for each field, having separate methods would make that behavior clearer.

[alextran1502]

That was my original idea: to split the suggestion into separate endpoints. It will be more code but clearer

[jrasm91]

I disagree, we don't need one endpoint for every select distinct query we plan to make

[jrasm91]

Maybe we can move the type part from a query param to a route param

[danieldietzler]

I disagree, we don't need one endpoint for every select distinct query we plan to make

So you're also in favor of one endpoint that returns an object of all suggestions for all fields, as I've described in Discord?

[mertalev]

This is creating a whole new array for each entry. This would be better

suggestions.country.push(...data.map(country => ({ label: country, value: country }))

If you can make the SearchSuggestionType enum values match the properties of suggestions, you can also fold all of this into:

suggestions[type].push(...data.map(entry => ({ label: entry, value: entry }))

[alextran1502]

This will not work with Svelte's reactivity

[danieldietzler]

True, but you could do

suggestions.country = [...suggestions.country, ...data.map(country => ({ label: country, value: country }))]

at least. Alternatively, suggestions could be writable and then you could trigger suggestions.update

[alextran1502]

Yeah, that one-liner is good. If we have to make a global state, the filter's value would be better, so we can modify the filter when showing the search result page.

[mertalev]

This code is difficult to understand. The first part removes the id from the filtered people if it exists and the second part adds it if it doesn't. So it's like a toggle? And filter.people is used in sortPeople to show selected people first and for styling. Is there a significance to finding the person entry as a whole and adding it to filter.people instead of using the id?

I think it'd be a bit clearer (and faster) if you made filter.people a set. Renaming the filter object to selected would also make the intent more explicit. And if you changed sortPeople to showSelectedPeopleFirst, it'd convey what it's sorting by.

  const handlePeopleSelection = (id: string) => {
    if (selected.people.has(id)) {
      selected.people.delete(id);
    } else {
      selected.people.add(id);
    }
    showSelectedPeopleFirst();
  };

[alextran1502]

The value in the array is PersonReponseDto, it is an object so it doesn't really work with Set. We will need the object to render the name and thumbnail in the people list

[mertalev]

filter.people as it is only seems to be used to check if a person ID exists in it. I don't think it needs to have the person object itself for this. It can just be a set of IDs. suggestions.people is what's used to get the name etc. in the current code.

[jrasm91]

This looks good to me, with the exception of the api design for the search suggestion endpoint. I think we should have more internal discussion around the final design before merging it. I'm not a fan of one endpoint returning all values for all fields at the same time, but I agree with mert that some separate endpoints would make the validation more meaningful, so I have some more ideas about this.

[waclaw66]

Bugs:

• X button covers new filter button when some search string is entered...
  
• filter window cannot be closed clicking outside
• search button doesn't work, it does nothing
• using Enter to confirm search reloads the whole page on /photos?context=panorama&start-date=&end-date=&radio-type=all without any result - regular photos page
• using new filter from the results returned by old simple search, reloads the page always with no results with message No results Try a synonym or more general keyword

browser: Firefox, Chrome