Add new Button on Course Page to Add Course to Planner #3731
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
@EssWhyy is attempting to deploy a commit to the modsbot's projects Team on Vercel. A member of the Team first needs to authorize it. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #3731 +/- ##
=======================================
Coverage 53.62% 53.62%
=======================================
Files 272 273 +1
Lines 5979 6020 +41
Branches 1428 1437 +9
=======================================
+ Hits 3206 3228 +22
- Misses 2773 2792 +19 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Have verified that this works! Thanks for your contribution.
The code looks good, full props (pun intended) for following the current style of the surrounding components.
This is completely optional, but it would be a nice-to-have if we could create new components in the functional style instead of class style — I would like to slowly move the codebase towards this pattern.
The specific code comments are just nits, and I would still merge them in if you don't touch the code referenced in those comments.
I'm requesting changes only because of the component name, which I believe could be more specific than SaveModuleButton. Something that references the planner / plan-to-take action might be better.
| for (let i = 0; i < planToTakeModules.length; i++) { | ||
| if (planToTakeModules[i].moduleCode === module.moduleCode) { | ||
| return true; | ||
| } | ||
| } |
There was a problem hiding this comment.
Is it possible to use a .some call here instead?
e.g. planToTakeModules.some(m => m.moduleCode === module.moduleCode)
| for (let i = 0; i < planToTakeModules.length; i++) { | ||
| if (planToTakeModules[i].moduleCode === module.moduleCode) { | ||
| return planToTakeModules[i].id; | ||
| } | ||
| } |
There was a problem hiding this comment.
Is it possible to use a .find call here instead?
e.g. planToTakeModules.find(m => m.moduleCode === module.moduleCode)?.id ?? '0'
| const PLAN_TO_TAKE_YEAR = '3000'; | ||
| const PLAN_TO_TAKE_SEMESTER = -2; |
There was a problem hiding this comment.
Is it possible for us to import these constants from `src/utils/planner.ts' instead?
Or better yet, create a new function that more accurately represents what this code is trying to do, i.e., add a module so far into the future that it will definitely be in the "plan to take" section (correct me if this not what the code is trying to do)?
| .dropdownItem { | ||
| text-align: center; | ||
| white-space: normal; | ||
|
|
||
| br { | ||
| display: none; | ||
| } | ||
|
|
||
| strong { | ||
| font-size: 1em; | ||
| } | ||
| } |
| :global(.dropdown-toggle.dropdown-toggle-split) { | ||
| width: 2rem; | ||
| border-left: none; | ||
|
|
||
| @include media-breakpoint-down(sm) { | ||
| // Long selector for specificity | ||
| width: 3rem; | ||
| } | ||
| } |
There was a problem hiding this comment.
Global seems like quite a large scope, do you know if this styling is actually applied on the button?
Context
Implementation for #3399
Implementation
Added a button to each module page to allow a user to save the module to the planner in the Plan to Take section, acting like a save button of sorts.
If it is already added to Plan To Take, the button will remove the module from there instead.
Test Run:
31.05.2024_16.53.21_REC.mp4
Other Information