-
Notifications
You must be signed in to change notification settings - Fork 18
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
feat: add course reset tab to learner's information #376
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #376 +/- ##
==========================================
+ Coverage 88.10% 88.34% +0.23%
==========================================
Files 162 164 +2
Lines 3380 3448 +68
Branches 836 850 +14
==========================================
+ Hits 2978 3046 +68
Misses 398 398
Partials 4 4 ☔ View full report in Codecov by Sentry. |
}, | ||
]} | ||
data={renderResetData} | ||
styleName="custom-table" |
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.
is this needed?
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.
Apparently, this is needed for the table style. I'm sorry for the confusion
src/users/CourseReset.jsx
Outdated
return () => clearInterval(intervalId); | ||
}, [courseResetData]); | ||
|
||
const handleSubmit = async (courseID) => { |
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.
write it with useCallback
.
src/users/CourseReset.jsx
Outdated
close(); | ||
}; | ||
|
||
useEffect(() => { |
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.
this useEffect
should be able to combine.
useEffect(() => {
fetchData();
if (data)
...(above code)
else(error)
}, []);
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.
isMounted
is unnecessary because []
already make sure it run only once.
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.
Can you explain what you mean by this useEffect should be able to combine.
, please?
For the isMounted
part, I was getting a warning when running test which is why I added it.
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 👍
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.
Haven't tested this locally but the changes look good to me.
Summary
https://2u-internal.atlassian.net/browse/AU-1845
Adds Course Reset tab to Learner's Information.