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

[FLOW-23] Enable Professor Reviews to Select From All Professors #189

Merged
merged 7 commits into from
Feb 13, 2025

Conversation

jerryzhou196
Copy link
Collaborator

@jerryzhou196 jerryzhou196 commented Nov 23, 2024

demo.mp4

Funny enough, this was far less trivial of a change than I anticipated. Basically, after loading all professors into the original dropdown widget, I would get this insane lag that would render the course review essentially unusable.

With the profiler, one character key-input would take 270% longer after loading the ~3,000 professor names in the dropdown, shown below. To fix this, I rewrote the src/components/input/DropdownList.tsx component to use a virtualized view using react-window plus using our existing fuzzy-sort library which made it much smoother.

benchmark comparison (1)

This pull request includes multiple changes to the package.json file and various components to improve the functionality and performance of the application. The most important changes include updating dependencies, modifying the CourseReviewBox component to handle a new query, and enhancing the DropdownList component.

Dependency Updates:

  • Added @types/react-window and react-window dependencies to package.json. [1] [2]
  • Removed react-custom-scrollbars and its types from package.json. [1] [2]

CourseReviewBox Component Enhancements:

  • Imported ONLY_PROF_QUERY in CourseReviewBox.tsx to fetch all professors.
  • Added allProfs prop to CourseReviewBoxContent and utilized it for professor selection. [1] [2] [3]
  • Updated the DropdownList options to include all professors and adjusted the selection logic. [1] [2]
  • Added a new query for fetching all professors and integrated it into the CourseReviewBox component. [1] [2]

DropdownList Component Enhancements:

  • Replaced react-custom-scrollbars with react-window for better performance in DropdownList.tsx.
  • Updated DropdownList to use a fixed width and improved the filtering and rendering of options. [1] [2] [3] [4] [5] [6]

ProfileDropdown Component:

  • Set a fixed width for the DropdownList in ProfileDropdown.tsx.

GraphQL Query Updates:

  • Added ONLY_PROF_QUERY to fetch all professors in Prof.tsx.

2. Rebuilt dropdown widget to use virtualized view + fuzzy sort
- Removed custom scrollbars in favour of react-window to support viewing 3k+ prof entries
@jerryzhou196 jerryzhou196 self-assigned this Nov 23, 2024
Comment on lines -42 to +43
width = 'fit-content',
width = 150,
Copy link
Member

Choose a reason for hiding this comment

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

Could this break other dropdowns? Should we keep width fit-content and just set minWidth in addition?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, this was really sad - I had to manually check each of the dropdown since for some reason react-window doesn't support the min-width attribute which is such a random shortcoming. I'll see if I can override the CSS manually, but odds are we might have to go with this one

@@ -80,6 +80,7 @@ const ProfileDropdown = () => {
</Query>
<DropdownList
selectedIndex={-1}
width={130}
Copy link
Member

Choose a reason for hiding this comment

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

May not need this if we keep fit-content and set minWidth instead for the other dropdowns?

@jerryzhou196 jerryzhou196 merged commit 6573d46 into main Feb 13, 2025
1 check passed
@jerryzhou196 jerryzhou196 deleted the flow-23-enable-all-professors-review branch February 13, 2025 17:59
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