-
Notifications
You must be signed in to change notification settings - Fork 11
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
chore: normalize search #942
Conversation
[diff-counting] Significant lines: 18. |
Visit the preview URL for this PR (updated for commit b9538d8): https://cornelldti-courseplan-dev--pr942-simon-search-normali-mihu65yb.web.app (expires Mon, 28 Oct 2024 02:20:53 GMT) 🔥 via Firebase Hosting GitHub Action 🌎 Sign: 6d4e0437c4559ed895272bbd63991394f1e0e933 |
code.push(course); | ||
} else if (course.titleLong.toUpperCase().includes(searchText)) { | ||
} else if (courseTitle.includes(normalizedSearchText)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you fixing this too, so now if you write "introtocomputing" (or anything other typing result), results for "intro to computing" will show up
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the fix! tested with different course codes with and w/o spaces - works on my end as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tested out the search text with different number of whitespaces, and it works for both the course code and the course title.
Can we also change the code atp so that if the user types both the course code and title, courseplan also shows the valid courses? right now if you write cs 1110: intro..., it will show nothing, but separately writing "cs 1110" and "introduction to computing" will show |
Def something we can look at in the future - i think for now doing it with course number as one possible search and course title as one possible search will work... especially because if they start typing cs 1110, they'll see the course they need before they finish typing anyway. but definetely something we can look at in the future |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thank you for this fix simon!! approved :)
Implemented! |
We now ignore whitespace (in addition to case) when searching for a new course in the course selector. You can confirm this by, on this branch, typing in e.g.
cs1110
and noticing thatCS 1110: ...
gets matched.