-
Notifications
You must be signed in to change notification settings - Fork 91
feat: Missing Sections Warning #1376
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
Conversation
KevinWu098
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.
Some misc. comments — I'll take another look once they're addressed and once the merge conflicts with main are resolved
apps/antalmanac/src/components/RightPane/SectionTable/SectionTable.tsx
Outdated
Show resolved
Hide resolved
apps/antalmanac/src/components/RightPane/CoursePane/CourseRenderPane.tsx
Outdated
Show resolved
Hide resolved
apps/antalmanac/src/components/RightPane/AddedCourses/AddedCoursePane.tsx
Outdated
Show resolved
Hide resolved
… deployment test works
KevinWu098
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.
Looking really good. Mostly minor comments, except one longer set of comments regarding sectionTypes on load that (thankfully) is non-blocking.
However, I believe this PR introduces a bug when we undo an action on AA, which crashes the page. You can reproduce the error easily. I believe this may stem from our use of Set, which doesn't serialize well, so we may need to use an array as the type instead (this should be easy via gpt)
Sorry for all the back and forth!
apps/antalmanac/src/components/RightPane/CoursePane/CourseRenderPane.tsx
Outdated
Show resolved
Hide resolved
|
Also, just curious @calebwongg. What is the added fix / change in this PR that makes it not break like the previous attempt? |
Making section types optional prevents the original reason why it broke: sectionTypes?: Set; When a user initially loaded in their saved classes, section types were not enriched when the page was loaded in, so my getMissingSections was looking for an attribute that didn't exist. |
hm... that's really interesting since you'd imagine that there would be a type error... perhaps related to #1372? |
…serilization unpredictable behaviors
Just did some further digging/clarifying... I was wrong this was actually NOT what caused the break...it was the line I added section types in the declaration for courseDetails in ` for (const course of department.courses) { ` |
|
/deploy |
KevinWu098
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.
LGTM — thanks for sticking with this!
Summary
Added a warning displaying the missing sections for a course on the added courses page.
When a user selects classes for a course, the required sections for a course are checked (e.g., lecture, lab, discussion) have been added. If any required section type is missing, the UI displays a warning so the user can correct their schedule.
Test Plan
Issues
Closes #1330
Before
After
Future Followup
Notes