-
Notifications
You must be signed in to change notification settings - Fork 345
Add Optimiser Unit Tests #4094
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
Add Optimiser Unit Tests #4094
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
💡 Enable Vercel Agent with $100 free credit for automated AI reviews |
|
@leslieyip02 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 Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #4094 +/- ##
==========================================
+ Coverage 54.52% 55.13% +0.60%
==========================================
Files 274 297 +23
Lines 6076 6724 +648
Branches 1455 1617 +162
==========================================
+ Hits 3313 3707 +394
- Misses 2763 3017 +254 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
jloh02
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 good refactor and tests
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.
Changes look good but i'm not sure if it's a refactor or latest merge changes. The tooltip (i) is broken and there are font changes (Time dropdown text is significantly larger). Not sure if these are intended. Just merged master in to this and hopefully some of these are fixed. If not, let's try to keep the fonts more uniform
|
Tooltips still broken :( |
@thejus03 I standardized the dropdown sizes so that the dropdowns look more uniform. However, if we add more dropdowns in the future, we can always customize their widths. |
225a4e9 to
e119719
Compare
website/src/views/optimiser/OptimiserForm/OptimiserFreeDaySelect.tsx
Outdated
Show resolved
Hide resolved
|
|
||
| <div className={styles.freeDaysButtons}> | ||
| {days.map((day) => ( | ||
| <button |
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.
That's okay but you might consider using a proper checkbox instead https://getbootstrap.com/docs/4.6/components/forms/#custom-forms
website/src/views/optimiser/OptimiserForm/OptimiserTimeRangeSelect.tsx
Outdated
Show resolved
Hide resolved
website/src/views/optimiser/OptimiserContainer/OptimiserContent.tsx
Outdated
Show resolved
Hide resolved
website/src/views/optimiser/OptimiserContainer/OptimiserContent.tsx
Outdated
Show resolved
Hide resolved
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.
|
The UI changes were made to address @ZhangYiJiang's comments on accessibility. The free days select should be OK, but I think I'll push a commit to make the dropdown fit in better with the rest of the form (I hadn't checked Safari, but it looks a bit jarring). Update: I've updated the dropdown to use
|







Context
Closes #4092.
Legacy Issues
Closes #4095.
Closes #4102.
Implementation
typesfolderutilsfolder + unit testsOptimiserFormcodeNotes
Replaced
uniqueKeywithlessonKeyPreviously, we had:
uniqueKey=>{moduleCode}-{lessonType}displayText=>{moduleCode} {lessonType}lessonKey=>{moduleCode}|{lessonType}I replaced
uniqueKeybecause I think the deduplication makes the code a bit easier to follow.useOptimiserFormcustom hookWe had a lot of
useStatecalls inOptimiserContent, and they were being passed toOptimiserFormas well. I think having a custom hook makes it easier to pass around, and allowed to pass props down to individual form fields, e.g.: