209 searchfilter bar - Githubissues

vercel[bot] commented 10 months ago

The latest updates on your projects. Learn more about Vercel for Git ↗︎

1 Ignored Deployment

cloudflare-pages[bot] commented 10 months ago

Deploying with Cloudflare Pages

Latest commit:	`670c011`
Status:	✅ Deploy successful!
Preview URL:	https://ef9d1899.correlaid.pages.dev
Branch Preview URL:	https://209-searchfilter-bar.correlaid.pages.dev

View logs

jstet commented 10 months ago

@friep @KonradUdoHannes as this is a complex new feature, it would be cool if you could test it and give some feeback

KonradUdoHannes commented 10 months ago

@jstet I'll review it tomorrow. What is the scope of the filtering that we want to cover in this PR? As far as I can tell by a quick view this focusses on Events, but I wonder how complex/simple we want the filter options to be? #209 does not appear to be very explicit about that yet.

jstet commented 10 months ago

My idea was to write a filter component that we can use on all pages where it makes sense e.g. projects Posts etc. Filter options then depend on tpye of Filtered data

friep commented 10 months ago

i can't give a technical review (thanks @KonradUdoHannes for the thorough review!!) but a few remarks:

i'd delete the : after the type etc.
maybe city and type could be multi select filters like tags?
on mobile modifying any of the filters creates a "cursor" and opens my keyboard which feels unnecessary / annoying
also on mobile, if i want to select multiple tags, i have to open the filter option every time. is there a way to make this a "i open this once and select all i want" thing?
do we have to have the dropdown to open the filters proactively?
under "filter" there is a horizontal line. i'd expect this line to separate the filters from the events but instead it's separating "filter" from the filter options. or is this intentional?

jstet commented 10 months ago

i can't give a technical review (thanks @KonradUdoHannes for the thorough review!!) but a few remarks:
* i'd delete the `:` after the type etc.
Done

maybe city and type could be multi select filters like tags? For city i agree, but an event can only be of one type so far.

on mobile modifying any of the filters creates a "cursor" and opens my keyboard which feels unnecessary / annoying will try to change this

also on mobile, if i want to select multiple tags, i have to open the filter option every time. is there a way to make this a "i open this once and select all i want" thing? will try to change this

do we have to have the dropdown to open the filters proactively? Personally i like this as people wont always want to filter and this doesnt block looking at the events (especially on mobile)

under "filter" there is a horizontal line. i'd expect this line to separate the filters from the events but instead it's separating "filter" from the filter options. or is this intentional? Yes this was a design choice by me. I think i saw filter designs like this before and wanted to recreate it. Personally i like it.

jstet commented 10 months ago

* on mobile modifying any of the filters creates a "cursor" and opens my keyboard which feels unnecessary / annoying
will try to change this

This is a matter of modifying searchable={true} in the props of the select component. I set it to false everwhere but for tags
* also on mobile, if i want to select multiple tags, i have to open the filter option every time. is there a way to make this a "i open this once and select all i want" thing?
will try to change this

The way this filter currently works is that it immediately applies the selection and also changes the url. theres the option closeListOnChange={true} which would keep the list open. However, as the selection changes, the list is closed after selecting. An alternative i can think of is using a submit button that causes the selection to be applied.

jstet commented 10 months ago

I think 1. is very good, but I'm wondering whether we should restrict the possible filters until the design is a little more settle to reduce refactoring efforts. Generally I think having several categories in mind is good for thinking about how this scales to filtering other collections instead of events. Currently the categories are tightly coupled to the functionality and I thing we would benefit from decoupling this a little bit.

yea the goal is decoupling based on filter_type. I will adjust the component when applying this to other data. Regarding 2. I think this is nice functionality, but I wonder how important that requirement is for us, as it also increases complexity quite a bit.

i expect a filter to work with search params. example: you filter by lcs and go to an event. then you go back to the filter but you would have to filter again if we wouldnt use search params. Regarding 3, this is actually what I would have expected as a filter. I'm not sure how widely spread that opinion is, but I think we should think about it at this stage to avoid situations where we later realize it would be nice, but it no longer fits in the overall design. Instead of a dropdown for tags, we could use a free search field that also considers tags. I agree that this would be nice but it would also add complexity i guess.

Implementation

Parsing and filtering

I think this is currently a little mixed up because it happend at the same time. But I think we should try to straighten it out such that parsing happens server side and filtering on the client side. We should also iron out parsing issues as we com across them (for instance parsing lc city names outside the parsing functionality).

Fitlered data store vs bind variables

I would suggest to use a bind variable instead f a store for the filtered data. This should make the code more local and more readable.

Yes i adjusted it to work w/o store

Will address rest of comment later

jstet commented 10 months ago

@friep and @KonradUdoHannes I reworked the filter component based on your input. It also supports free text search now.

It would be awesome if you could test my changes.

jstet commented 10 months ago

[x] Check accessibility

friep commented 10 months ago

mobile UX is so much better 🔥

maybe it would be worthwhile to introduce arguments that controls whether a filter is multiselect or single-select and whether it's an "AND" or an "OR" if multiselect (with OR being default). Independent of the data model in directus.

example:

event can only be one type but the user might be interested in workshops and talks (multiselect, OR)

jstet commented 10 months ago

maybe it would be worthwhile to introduce arguments that controls whether a filter is multiselect or single-select and whether it's an "AND" or an "OR" if multiselect (with OR being default). Independent of the data model in directus.

example:
* event can only be one type but the user might be interested in workshops and talks (multiselect, OR)

I will create a new issue to implement the OR functionality in the future

jstet commented 10 months ago

While testing a local deployment in the browser I noticed a few translation issues with the labels of the selectors. It seems that only the label of the search selector gets updated reactively and the other ones do not. Other than that I did not notice any issues.

fixed

CorrelAid / correlaid_website

209 searchfilter bar #484

Deploying with Cloudflare Pages

Implementation

Parsing and filtering

Fitlered data store vs bind variables