Couchers-org / couchers

The next-generation couch surfing platform. Free forever. Community‑led. Non‑profit. Modern. Chuck us a star :)
https://couchers.org
MIT License
389 stars 79 forks source link

Filter option #4853

Closed bakeiro closed 1 month ago

bakeiro commented 1 month ago

before: image

now: image

closes: https://github.com/Couchers-org/couchers/issues/4799

Backend checklist

Web frontend checklist

vercel[bot] commented 1 month ago

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

Name Status Preview Updated (UTC)
couchers ✅ Ready (Inspect) Visit Preview Sep 23, 2024 9:10pm
nabramow commented 1 month ago

Okay apologies I didn't have a chance to really dive into this until now and understand what was happening fully.

I think you have the right idea here but there's a lot changing at once and I think it's making your life more difficult.

What I would do is break it into small pieces:

  1. Start a new PR with just one of these filters, for example JUST 'hostingStatusFilter' and get that working with the types and the multi-select (from my understanding the main thing in this ticket is add multiple selection).
  2. Don't remove the value type inSelect. It seems you're removing it just to fix Typescript, but since this Select is used elsewhere, that increases the chance of a bug. Instead you want to add an array value to the value type, see #4881 .

What I see is you want the value to be a number and the label to be a string. Think of the types in that way.

The types here are not totally correct, and since so much changed at once it's hard to fix. This is why I would suggest starting a new PR to just do one first. Get the logic working without coercing the types (i.e. no as number, as string, etc.), add the design later. You can use the Material UI multiselect example here:

https://mui.com/material-ui/react-select/#multiple-select

Do a feature branch if you need to. Then it can be one PR to review the logic, then one PR to review design.

If it's easier, you can make a new shared MultiSelect component, rather than change the existing one.

I think it's a bit dangerous to merge this as is, as you're coercing the types as number[], etc. when that's not really what they are. This could cause the app to error out.

See example here: https://github.com/Couchers-org/couchers/pull/4881

Feel free to take over that PR, or copy from it and then close it.

bakeiro commented 1 month ago

splitting in 2 PRs

https://github.com/Couchers-org/couchers/pull/4884 https://github.com/Couchers-org/couchers/pull/4881