-
Notifications
You must be signed in to change notification settings - Fork 0
feat(web): degree progress chart #54
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
base: main
Are you sure you want to change the base?
Conversation
a32af93 to
0b3fcdf
Compare
chenxin-yan
left a comment
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.
see comments
| > = useQueries(programQueries); | ||
|
|
||
| // Collect all unique course codes from all programs | ||
| const allCourseCodes = new Set<string>(); |
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.
you are collecting all the course codes, but you should be treating courses with different types separately ("alternative", "required", "options")
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.
fixed. i now only fetch required and alternative courses as courses with type option already come with the credits attribute. the logic to deal with them separately is in degree-charts.tsx
| { credits: number; courses: string[][] } | ||
| > = {}; | ||
|
|
||
| // FIXME: currently no merging of programs if more than one listed (dual degree), assumes only one program for now |
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.
when getting all the courses form the programs that belongs to student, create a map that maps the program code with the actual program record so we can display program name propertly
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.
i'm not entirely sure what you mean by that. i currently already display the program name in the chart. are you saying that if a certain user has multiple programs (e.g. Computer Science B.A. and Economics B.A.), I should display it like "Program 1 and Program 2"? if that's the case i'll need to implement logic to merge the requirements of both programs. let me know what you think.
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.
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.
ohh okay okay will fix right now
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.
implemented hard coded map, not sure if there's a better way to do it
551a188 to
78a92a9
Compare

📌 What’s Changed
✅ Actions
📝 Notes for Reviewer