terror / mcgill.courses

A course search and review platform for McGill University
https://mcgill.courses
Creative Commons Zero v1.0 Universal
32 stars 8 forks source link

Client side fuzzy search on course reviews #526

Open Sergio-Na opened 5 months ago

Sergio-Na commented 5 months ago

Hey 👋🏻 , Pushing up what I've got so far for this feature. It's not fully tested and still needs some cleaning up, but I'd appreciate any early feedback you might have!

Resolves #377

39bytes commented 5 months ago

Hi, thanks for the PR. This issue was created a long time ago and the suggestion to use fuse was before we discovered flexsearch (which we use for client side course search), I'd suggestion switching this to also use flexsearch so we don't have 2 search libraries in the same project (unnecessarily increases bundle size, and also flexsearch is like a million times faster than fuse)

Another thing I'm not sure about is that client side search could potentially be incompatible with review pagination whenever we decide to implement that.

@terror @SamZhang02 thoughts?

SamZhang02 commented 5 months ago

Yeah I agree, sorry for the old and a bit confusing issue. Fortunately I vaguely recall them being pretty similar, so I think it shouldn't be a super hard swap.

I'll take a look at the rest in the next couple of days once I am done my finals :D

SamZhang02 commented 5 months ago

Hey Sergio, I hope your finals went well; Sorry for the wait, I kinda just slept through the entire week after the finals. Apart from the previous comment about using flexsearch, just a couple more comments Maybe we can use the searchbar UI from the explore page instead of the homepage one, it matches the components surrounding it a bit better than the current one in your branch, which is maybe a bit too strong visually

CleanShot 2024-05-02 at 13 03 25@2x

This one is a bit rounder and easier on the eye. We can perhaps also match the highlighted text from the Explore page as well for consistent behaviour (underline instead of red text)

There is also a small bug that the review searchbar currently doesn't clear after switching into another course's page, it should be cleared.

SamZhang02 commented 5 months ago

@39bytes @terror in this branch, searching reviews alters the rating display because its kind of applying a filter on it, do you anticipate this to be an issue?

https://github.com/terror/mcgill.courses/assets/112342947/e2e874a4-541f-4cc9-a323-52b3f490df10

I find it may be a bit confusing

39bytes commented 5 months ago

@39bytes @terror in this branch, searching reviews alters the rating display because its kind of applying a filter on it, do you anticipate this to be an issue?

https://github.com/terror/mcgill.courses/assets/112342947/e2e874a4-541f-4cc9-a323-52b3f490df10

I find it may be a bit confusing

This should not happen, too jank

SamZhang02 commented 5 months ago

@39bytes @terror in this branch, searching reviews alters the rating display because its kind of applying a filter on it, do you anticipate this to be an issue?

CleanShot.2024-05-02.at.13.07.38.mp4

I find it may be a bit confusing

This should not happen, too jank

@Sergio-Na if you can address this as well it would be great, thanks!