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

Consider sub-requirement progress in add modal choices #351

Merged
merged 4 commits into from
Mar 11, 2021

Conversation

SamChou19815
Copy link
Contributor

Summary

This PR addresses the problem mentions in this PR comment, where the current add modal choices completely disregards whether adding a course can help increasing the requirement fulfillment progress. This PR implements the proposal I mentioned in the follow-up PR comment, in which we will compute a new stat with the course added, and check whether it can increase the sub-requirement progress. If not, then the requirement won't show up as a choice.

To be able to call the function, we must create a CourseTaken object with real credits information. Therefore, we must generate that info when we are selecting the courses, so we must use the full course json instead of the simplified one. I switched the course selector to use the full course json, and after that the simplified course json becomes unused, so I deleted it.

Test Plan

Now the bug is fixed (when I have two math classes, adding one more math class won't cause the MQR requirement to show up):
Screen Shot 2021-03-07 at 15 55 41

@SamChou19815 SamChou19815 requested a review from a team as a code owner March 7, 2021 21:08
@dti-github-bot
Copy link
Member

dti-github-bot commented Mar 7, 2021

[diff-counting] Significant lines: 185.

@github-actions
Copy link
Contributor

github-actions bot commented Mar 7, 2021

Visit the preview URL for this PR (updated for commit c9ed704):

https://cornelldti-courseplan-dev--pr351-add-modal-choices-co-0bji223x.web.app

(expires Tue, 16 Mar 2021 00:24:25 GMT)

🔥 via Firebase Hosting GitHub Action 🌎

@SamChou19815 SamChou19815 force-pushed the add-modal-choices-considering-sub-req branch from e5fdbd2 to dd4230f Compare March 7, 2021 23:00
Copy link
Collaborator

@tcho6319 tcho6319 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks Sam! I left some comments below:

  1. I would check out NewSelfCheckCourseModal.vue b/c I think deleting courses.json might have had a side effect for that component.
Screen.Recording.2021-03-08.at.5.28.05.PM.mov

Comment on lines -20 to -21
{{ matchingCourse.title }}
<input type="hidden" :value="matchingCourse.title" :name="matchingCourse.roster" />
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think <input> here was for accessibility purposes. Is it ok to remove?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's not. It was there back when we have DOM manipulation and we use the input to read out the data we need instead of using emit.
The type is hidden, so the semantics is that it will also be hidden when viewed by the accessibility software, so there should be no impact on a11y.

@SamChou19815
Copy link
Contributor Author

@tcho6319 fixed the broken self-check add modal and added some doc for full course json

@SamChou19815 SamChou19815 requested a review from tcho6319 March 9, 2021 00:22
Copy link
Collaborator

@tcho6319 tcho6319 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Ty Sam 😀

@SamChou19815 SamChou19815 merged commit ba5c771 into master Mar 11, 2021
@SamChou19815 SamChou19815 deleted the add-modal-choices-considering-sub-req branch March 11, 2021 18:23
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.

3 participants