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

Filter by prof on course page #494

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

Filter by prof on course page #494

wants to merge 3 commits into from

Conversation

mjaydenkim
Copy link
Contributor

@mjaydenkim mjaydenkim commented Dec 24, 2024

Summary

This PR works on the filter by professor course page dropdown.

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/702f3af8-4f84-4440-ae53-e0308b45ea3a
image

QA - Test Plan

Checked on different screen sizes, fixing styles as needed. Looks slightly awkward on mobile since the header brushes up against the text, but they never overlap. Checked on a variety of classes to ensure that all professors were added to the dropdown.

Breaking Changes & Notes

Added to documentation?

  • πŸ“œ README.md
  • πŸ““ notion
  • πŸ• ...
  • πŸ“• ...
  • πŸ™… no documentation needed

What GIF represents this PR?

gif

Known problem: At very small dim, filter overflows beyond background
@mjaydenkim mjaydenkim requested a review from a team as a code owner December 24, 2024 06:32
@CLAassistant
Copy link

CLAassistant commented Dec 24, 2024

CLA assistant check
All committers have signed the CLA.

@dti-github-bot
Copy link
Member

dti-github-bot commented Dec 24, 2024

[diff-counting] Significant lines: 91.

@mjaydenkim mjaydenkim changed the title Matt/filter by prof Filter by prof on course page Dec 24, 2024
Copy link
Contributor

@qiandrewj qiandrewj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This works super cleanly, Matt! It's great now that we have multiple sorting/filtering methods that work seamlessly with each other. Left a few comments, but thanks :)

<h2 className={styles.title}>Past Reviews ({courseReviews?.length}) </h2>
<h2 className={styles.title}>
Past Reviews (
{selectedProf.current !== 'none'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's more intuitive for me to have it as just visibleCourseReviews.length because otherwise, you might interpret it as being on a second "page" of reviews or something. Can have others give feedback too

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh wait this makes a lot of sense!! my logic behind this was that i wanted a way to make sure that users easily know that when a professor is selected they're not seeing all the reviews (esp b/c the statistics on the left sidebar are based on all reviews). i think just changing the number itself works though!

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