feat: add allowance modal #36
Conversation
| setForm(prev => ({ ...prev, 'exam-type': defaultExamType })); | ||
| handleExamTypeChange(); | ||
| } | ||
| }, [examsList]); // eslint-disable-line react-hooks/exhaustive-deps |
There was a problem hiding this comment.
I'm struggling to test this component because it seems like the useEffect isn't running when I render the component in a test. If I take this code out of the useEffect, it causes too many renders. Any suggestions?
There was a problem hiding this comment.
Nothing stands out to me about the hook that would cause it not to run. Have you confirmed that it's not running, or is it running and not doing anything? Maybe there's an issue with your set up (e.g. examsList is empty)?
However, my initial thought is that this doesn't need to be in a useEffect hook, based on You Might Not Need an Effect. When you say "it causes too many renders" when you move it out of the useEffect hook, what do you mean by that?
If necessary, you could use the useMemo hook to memoize the results of the computation to avoid recomputing it unless examsList changes, but you'd have to refactor your hook a little to return something that can be memoized (e.g. the proctoredExams and timedExams variables).
What do you think?
There was a problem hiding this comment.
If I run tests when this code is outside of a hook, the tests fail because of too many re-renders. The line that's specifically causing too many rerenders is setForm(prev => ({ ...prev, 'exam-type': defaultExamType }));. If I comment that line out, the tests pass.
There was a problem hiding this comment.
I'll pull down your code and take a look. I'll follow up soon.
| additionalTime = form['additional-time-minutes']; | ||
| } | ||
|
|
||
| const userKey = user.includes('@') ? 'email' : 'username'; |
There was a problem hiding this comment.
I'm not sure if I should include a more strict check for email vs username.
There was a problem hiding this comment.
Can you explain why we need to differentiate email or username here? Only for display purposes?
There was a problem hiding this comment.
The backend accepts either email or username for the user identifier, so we need to specify which identifier we are sending from the frontend.
alangsto
force-pushed
the
alangsto/add_allowances_modal
branch
3 times, most recently
from
df8808e to
b5c4a9e
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## feature/add_allowances #36 +/- ##
=========================================================
Coverage ? 80.47%
=========================================================
Files ? 24
Lines ? 466
Branches ? 76
=========================================================
Hits ? 375
Misses ? 86
Partials ? 5 ☔ View full report in Codecov by Sentry. |
alangsto
force-pushed
the
alangsto/add_allowances_modal
branch
4 times, most recently
from
e8b9a1c to
1aa8132
Compare
alangsto
force-pushed
the
alangsto/add_allowances_modal
branch
2 times, most recently
from
e3dd98b to
ef04d23
Compare
alangsto
force-pushed
the
alangsto/add_allowances_modal
branch
from
ef04d23 to
0fedd80
Compare
COSMO-393
This PR adds a modal component that users may interact with to add allowances for specified users.
Default view:
Error view:
