Skip to content
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

Merged
merged 2 commits into from
Oct 2, 2024
Merged

Conversation

Destaq
Copy link
Member

@Destaq Destaq commented Sep 19, 2024

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 that CS 1110: ... gets matched.

@Destaq Destaq requested a review from a team as a code owner September 19, 2024 22:00
@dti-github-bot
Copy link
Member

dti-github-bot commented Sep 19, 2024

[diff-counting] Significant lines: 18.

Copy link
Contributor

github-actions bot commented Sep 19, 2024

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)) {
Copy link
Contributor

@plumshum plumshum Sep 22, 2024

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

Copy link
Contributor

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.

Copy link
Contributor

@plumshum plumshum left a 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.

@plumshum
Copy link
Contributor

plumshum commented Sep 22, 2024

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 cs1110

@nidhi-mylavarapu
Copy link
Contributor

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 cs1110

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

Copy link
Contributor

@nidhi-mylavarapu nidhi-mylavarapu left a 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 :)

@Destaq
Copy link
Member Author

Destaq commented Sep 28, 2024

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 cs1110

Implemented!

@nidhi-mylavarapu nidhi-mylavarapu merged commit 9dc23cd into main Oct 2, 2024
10 of 11 checks passed
@nidhi-mylavarapu nidhi-mylavarapu deleted the simon-search-normalization branch October 2, 2024 21:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants