-
Notifications
You must be signed in to change notification settings - Fork 14
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
Draft
Awesome-E
wants to merge
17
commits into
main
Choose a base branch
from
major-requirements
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
TODO: Add intermediate phase with something like `setActiveCourseLoading` for dragged courses
Add back a delay
Note: this currently does not work with new roadmaps
Awesome-E
requested review from
CadenLee2 and
timobraz
and removed request for
CadenLee2
February 5, 2025 07:50
CadenLee2
reviewed
Feb 7, 2025
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Screenshots
Test Plan
This PR is work in progress
Issues
Closes #568