-
Notifications
You must be signed in to change notification settings - Fork 15
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
Implement Course Requirements for Undergraduate Majors #578
Conversation
TODO: Add intermediate phase with something like `setActiveCourseLoading` for dragged courses
Add back a delay
Note: this currently does not work with new roadmaps
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.
Overall functionality
- Although majors don't save locally when not signed in, this doesn't seem like a major inconvenience (considering that a user hoping to work on their roadmap long-term would probably sign in)
- Because the course titles alone in the major requirements list don't show past quarter offerings, I think that makes it impossible to see quarter offerings on the desktop roadmap without using the All Courses/search menu. This probably increases the importance of our planned change of moving past quarter offering information to the popover
Styling Details
- Is the green color for completion of a group requirement finalized or just a temporary placeholder? I noticed it's just the default
green
and didn't see an example of what it should look like on the Figma, and it stands out a bit since there isn't much green elsewhere on the roadmap - Rare long course names like ANTHRO121AW still stick out slightly from their cells. You had mentioned a while ago that it doesn't scale font size "yet"--is this a goal, or will we keep the font sizes the same?
- Noticed that certain other course titles wrap (see CHC/LAT132B under History B.A. History spec)
- The major/spec dropdowns use a different blue color that doesn't match the rest of PeterPortal (is it from the react-select default style?). Additionally, in dark theme, the text on the selected blue item is black, as opposed to the white-on-blue we have elsewhere.
For the most part, the changes work well:
- Clearing requirement groups works
- Major saving to my account works, including with multiple roadmaps
- Tested various majors (with and without specializations); signed in and signed out; desktop and mobile; multiple roadmaps with different majors per roadmap
- Didn't fully inspect all the code closely, but at a glance the file structure (such as how the sidebar is organized) looks good and major changes make sense
* Added minor requirement selector * Switching roadmaps keeps the same minor
* feat: ge requirements list * doc: remove unnecessary comment * refactor: remove unnecessary async * Change "All Courses" to "Search" for now - This is required to make the text in the button fit on one line * border between buttons --------- Co-authored-by: Awesome-E <[email protected]>
- For nested groups that are complete, the completed subgroups satisfy the outer group and are green. - This commit changes styles for the groups that don't count towards completion for the outer group, and are thus ignored. - For those groups, instead of blue border + green strikeout text, it is now just gray border and gray text.
Marking as ready for review now, but will address the comment later |
Agree
Yes, it is intended as that for now. We can consider matching it to the green that is in the graph at a later point though if that helps with consistency.
This is more or less the default theme for
I will see if I can make a last-minute fix tomorrow? |
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.
Significant features work well
- Major (with and without spec), minor, GE requirements, and search tabs all show the right info; tracking completion works; works on mobile
- The collapsed-by-default requirement groups make requirements easier to manage/less overwhelming at first, especially when groups have a lot of courses, so I think this was a good change
Details
- I do notice that minor requirements reload each time you click the tab, even if the dropdown selection hasn't changed. Not a large problem, but could be nice (and probably not difficult) to make them not reload unless the minor has been changed
- Also, when logged in, major requirements reload each time you click the tab (because major/spec is loaded from the account each time). Also not necessarily a problem, and technically this could include desired behavior (if you have PeterPortal open in two tabs open at once and change the major on one, then switching back to the major tab on the other will update it). Could be nice to check whether the major/spec has changed, and if not, we don't need to reload major requirements (maybe in the future since we want this merged soon)
Misc.
- Same styling comments as the previous comment
(Edited because I originally thought this was a review on the most recent changes since my last view, not the full PR)
* Add selector between requirement tabs * Initial Course Tile Display * Make adding courses sorta work * Make Groups Collapsible * Make hovering less ridiculous (Add back a delay) * Load Major and Specs from API * fix component flashing * Preserve selection in global state * render same course list for unit reqs * Rename `RequiredCourseList` to `ProgramRequirementsList` * Temporary completion check * Wait for course to load before fully adding * Combo Box Dark Mode * Make Completion checks work for units * Fix bug with changing specs * collapse nested, open courses * Make selecting a major save to account - Note: this currently does not work with new roadmaps * Fix lint issue * Implement minor requirements (#595) * Added minor requirement selector * Switching roadmaps keeps the same minor * fix types * GE Requirements (#588) * feat: ge requirements list * doc: remove unnecessary comment * refactor: remove unnecessary async * Change "All Courses" to "Search" for now - This is required to make the text in the button fit on one line * border between buttons --------- Co-authored-by: Awesome-E <[email protected]> * Flatting singleton groups * Collapse all by default * move planner_minor to its own table * Update Style for Ignored subgroups - For nested groups that are complete, the completed subgroups satisfy the outer group and are green. - This commit changes styles for the groups that don't count towards completion for the outer group, and are thus ignored. - For those groups, instead of blue border + green strikeout text, it is now just gray border and gray text. * Fix minor using a shared function --------- Co-authored-by: Ellis Liang <[email protected]> Co-authored-by: Caden Lee <[email protected]>
Description
Screenshots
Test Plan
This PR is work in progress
Issues
Closes #568