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

Implement Course Requirements for Undergraduate Majors #578

Draft
wants to merge 17 commits into
base: main
Choose a base branch
from

Conversation

Awesome-E
Copy link
Member

Description

  • Splits the search sidebar in the roadmap to "Major" and "All Courses"
  • Provides the option to select from existing majors using data from the API
  • If the selected major has specializations, provides the option to select a spec
  • When a major (and spec if present) is selected, a required course list will be shown

Screenshots

image

Test Plan

This PR is work in progress

Issues

Closes #568

Note: this currently does not work with new roadmaps
Copy link
Contributor

@CadenLee2 CadenLee2 left a 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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Courses Should Have Strikethrough if they are present in the roadmap Show Required Courses for a Major
2 participants