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

Search page upgrades #500

Open
wants to merge 13 commits into
base: main
Choose a base branch
from
Open

Search page upgrades #500

wants to merge 13 commits into from

Conversation

mjaydenkim
Copy link
Contributor

Summary

This PR upgrades the search pages. It updates outdated React search page code to use hooks and functional components, fixes the weird spacing on the original search page, behaves more predictably when no reviews are available due to bad searches/filters, and includes overall style improvements. Please refer below for important notes.

PR Type

  • 🍕 Feature
  • 🐛 Bug Fix
  • 📝 Documentation Update
  • 🎨 Style
  • 🧑‍💻 Code Refactor
  • 🔥 Performance Improvements
  • ✅ Test
  • 🤖 Build
  • 🔁 CI
  • 📦 Chore (Release)
  • ⏩ Revert

Mobile + Desktop Screenshots & Recordings

https://github.com/user-attachments/assets/f7085e89-734a-4f3d-81e3-10870de92a8b
Note: I got rid of the "Search Results" header wrapping onto two lines since recording this.

QA - Test Plan

Did testing on every possible feature on the search page (eg. different combos of filters, all possible sort arrangements, etc).

Breaking Changes & Notes

  • When the search page loads, there are some minor cases of flickering -- sometimes the no-reviews screen will flicker for a second before reviews show up, and the size of the preview box will flicker a bit before showing the "no reviews yet" image for the first time. I spent a while debugging this and I think I know why it's happening vaguely but I figured it was taking too long so maybe we can get to it at school. But also I don't think it matters significantly -- it's a pretty small thing. Annoying though
  • Changed the CSS-only loader's styling so that it would be centered on the viewport in all scenarios. I hope that's OK, if it was set the way it was for a certain reason that'd be good to know.
  • Added a flag that can hide search results for future use. On mobile, the search result panel makes the website kind of unwieldy since it's stacked above the detail view, so you have to scroll back and forth to access and view multiple items in detail; might want to modify this eventually so that the mobile version shows one view at a time (search or detail). But I'm not sure of the best way to design that so I'm gonna hold off and only include the functionality for now.
  • Filter checkboxes still look kinda ugly but I'm not sure what else to do with it. Using a multi select feels like too much given that there aren't many options. Maybe design our own?
  • Sorting by "relevance" feels arbitrary here since it's just the course number. I think it'd make more sense to sort by number of reviews but I don't want to make all those queries so maybe I'll add that to the schema when the school year starts idk.
  • My PRs keep failing and i don't know why </333

Added to documentation?

  • 📜 README.md
  • 📓 notion
  • 🍕 ...
  • 📕 ...
  • 🙅 no documentation needed -- lowk not sure if this needs to go anywhere

What GIF represents this PR?

gif

Need to do a big more testing but I think this is good.
Don't forget to mention -- when there's no course possible due to filters, the preview card is deselected.
Still to do: Figure out how to handle the very long mobile page, fix sorting by relevance to number of reviews if possible, run it by everyone!
Also got rid of useless comments
@mjaydenkim mjaydenkim requested a review from a team as a code owner January 15, 2025 08:58
@dti-github-bot
Copy link
Member

[diff-counting] Significant lines: 738.

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.

2 participants