Assign Course to Placeholder Modal (Front-End) #645
Conversation
|
[diff-counting] Significant lines: 111. |
|
Visit the preview URL for this PR (updated for commit 7269d79): https://cornelldti-courseplan-dev--pr645-placeholder-modal-6jc5l3wp.web.app (expires Thu, 03 Feb 2022 20:41:01 GMT) 🔥 via Firebase Hosting GitHub Action 🌎 |
There was a problem hiding this comment.
Thanks @willespencer!
I feel like the add placeholder modal should be a separate component from the add self check modal. I also had some questions and concerns regarding this implementation.
| this.isPlaceholderModalOpen = false; | ||
| }, | ||
| assignCourse() { | ||
| // TODO @willespencer write the code that assigns a course to a placeholder |
There was a problem hiding this comment.
How were we going to implement assigning courses to placeholders? I thought we modeled placeholders after normal courses because we were going to replace them -- I don't know if this added complexity is worth it.
There was a problem hiding this comment.
Sorry - assign might be the wrong choice here! Yes the idea was to insert and replace a placeholder with a course in the courses list.
| // TODO @willespencer write the code that assigns a course to a placeholder | ||
| }, | ||
| getRequirementID() { | ||
| // TODO @willespencer get requirement ID from name and slot |
There was a problem hiding this comment.
Just double checking, do placeholders and requirements have a 1:1 mapping? Where is the name and slot stored?
There was a problem hiding this comment.
Requirement name and slot is in the firestore list of courses! Every requirement slot will have at most as many placeholders as courses that are needed to fulfill it, but could have fewer in cases of double counting.
| getRequirementLearnMore() { | ||
| // TODO @willespencer get requirement learn more link to show on add modal | ||
| return 'https://cornelldti.org'; | ||
| }, |
There was a problem hiding this comment.
We might want to refactor all of this displayed requirements information into its own component (not necessarily in your PR). I think it's pretty hacky as it is now. I also think it should be calculated at the add placeholder modal level, not in the placeholder component.
|
hey @willespencer , sorry I just saw this! everything looks good so far! thanks!
|
Summary
This pull request implements the front-end for the modal that opens to assign courses to placeholders to continue Templates development. Follow-up from #592.
NewSelfCheckCourseModalto handle placeholder assigning as wellRemaining TODOs:
Complete the requirements side of this modal in a later PR. This should be easier as real placeholder data is populated into a user's Firestore collection. Specifically...
Test Plan
Similarly to #592 - placeholder data has to be added to Firestore manually. Afterward...
Notes
@kkkehui could you take a look and let me know if this looks good? I think I closely followed the designs - but I did keep the placeholder text inside the input box the same as our other add modals for consistency and easier coding. Let me know what you think!
Additionally, This PR can be safely merged as long as the self check add modal remains unchanged, as real users cannot yet have placeholders!